(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?