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?