I say use Revision [2].
BUT -- instead of hiding the logic in Monitoring Service it should be
easily accessible to other code.
So I added a Duck-typed method to Config.
Please use this:
Config c = ...
if(c.isDas())
.....
===========================================
~/dev/bg/main/nucleus/admin/config-api> svn commit
Sending
config-api/src/main/java/com/sun/enterprise/config/serverbeans/Config.java
Transmitting file data .
Committed revision 62759.
On 9/23/13 1:50 PM, Bill Shannon wrote:
> (Note that your attachments were reversed.)
>
> REVISION[2] seems "more correct" since it's not dependent on the name,
> but I can't find any other code that uses AdminService.getType so I'm
> nervous about using it.
>
> On the other hand, I did find lots and lots of code that depends on
> the name "server-config", so it seems that REVISION[1] is the way to go.
>
> I'd still like to hear from others whether there's a "better" way...
>
> lvsongping wrote on 09/22/13 19:10:
>> Hi, Bill, Byron:
>> Cc: dev list:
>>
>> IMHO, The parent never return null because it will eventually reach a
>> Config every times. If the Config can be return the null occasionally, I
>> think it will be another issue about the glassfish but not this one. As to
>> the issue approved by Bill, I have attached two different patch:
>>
>> REVISION[1]: It will still compare the config name to assure whether the
>> node is DAS or instance, I have confirmed there's already other code that
>> checks for the name "server-config"( SecureAdminUpgradeHelper.
>> ensureNonDASConfigsReady).
>>
>> REVISION[2]: I have also changed the to compare the adminservice's type
>> instead of config's name so that it can check whether the node is instance
>> or DAS correctly every times.
>>
>>
>> I prefer the REVISION[1] as there's already other code that checks for
>> the name "server-config". Please give me some feedbacks if you have time.
>>
>> Thanks
>>
>> Regards
>> Jeremy Lv
>>
>>
>> -----Original Message-----
>> From: Bill Shannon [mailto:bill.shannon_at_oracle.com]
>> Sent: Saturday, September 21, 2013 8:12 AM
>> To: lvsongping
>> Cc: dev_at_glassfish.java.net; byron.nevins_at_oracle.com
>> Subject: Re: [Review request] About GLASSFISH-20696
>>
>> Comments below.
>>
>> lvsongping wrote on 09/20/13 03:27:
>>> -----Original Message-----
>>> From: Bill Shannon [mailto:bill.shannon_at_oracle.com]
>>> Sent: Thursday, September 19, 2013 3:55 AM
>>> To: dev_at_glassfish.java.net
>>> Cc: lvsongping; byron.nevins_at_oracle.com
>>> Subject: Re: [Review request] About GLASSFISH-20696
>>>
>>> + ConfigBeanProxy proxy = (ConfigBeanProxy)(event.getSource());
>>> + Config config;
>>> + if(proxy instanceof MonitoringService) {
>>> + config = (Config)proxy.getParent();
>>> + } else {
>>> + config = (Config)proxy.getParent().getParent();
>>> + }
>>> +
>>> + String name = config.getName();
>>> + // We think that DAS's config is always server-config.
>>> + if(name.equalsIgnoreCase("server-config")) {
>>>
>>>> Is is true that the DAS's config is required to be named "server-config"?
>>>> Does other code depend on that?
>>> The default config name will be "server-config" when you create
>>> the DAS, here's some detailed information below.
>>>
>>>> Are config names case-independent? I wouldn't think so.
>>> You are right, we can create a DAS but the config name is not
>>> "server-config", this is why I add a annotation here as " We regard
>>> that DAS's config is always server-config.", we do this changes
>>> because we regard the user will use the server-config to create the
>>> DAS rather than create another config name based on the server-config.
>>> For example: if we want to create a DAS to another config, we can follow
>> the following steps:
>>> 1). asadmin copy-config server-config new-config 2). Then we can
>>> change the host port about the new-config 3). Create a standalone
>>> instance specified the --config option as new-config and we can regard
>>> this instance as another DAS.
>>>
>>> As to this situation the DAS's config name is "new-config" rather than
>>> "server-config", The main reason why I attached the patch before to
>>> regard the DAS's config name as "server-config" because I think this
>>> situation will be happened quite a little so that we can ignore this
>>> situation in my option.
>>>
>>> If you need to take this situation into consideration, I will come up
>>> with another patch as soon as possible to take all of the situations
>>> into consideration.
>> I'd like to hear from Byron or others who know the code better.
>>
>> I wonder if there isn't already a method to determine "is this the DAS's
>> config?"
>>
>> If there's no such method, and there's already other code that checks for
>> the name "server-config", I suppose it's fine to do the same here.
>>
>>>> Why is the MonitoringService special? Shouldn't it just go up the
>>>> parent
>>> chain until it gets to something that's "instanceof Config"?
>>> Both of the revision as I have attached before and you have
>>> presented can be used to fix this issue. The reason why I use the
>>> MonitoringService is because the issue we need to fix is based on the
>>> monitoring. So I used the MonitoringService instead of "instanceof
>>> Config". I have replaced the related code as follows and tested the
>> function could run successfully:
>>> + ConfigBeanProxy proxy = (ConfigBeanProxy)(event.getSource());
>>> + while(!(proxy instanceof Config)){
>>> + proxy = proxy.getParent();
>>> + }
>>> + Config config = (Config)proxy;
>> That's better.
>>
>> Can getParent ever return null?
>>
>> Will getParent always eventually reach a Config?
--
---
All Generalizations are false. Including this one.