On 9/22/13 7:10 PM, lvsongping wrote:
> Hi, Bill, Byron:
> Cc: dev list:
>
> IMHO, The parent never return null
Always always always always always handle a reference possibly being
null. Why?
1. Even if it is impossible now (unlikely) that doesn't mean it will be
impossible in 2 years when Joe the Intern changes upstream code.
2. The reader of your code looks at that and thinks, like both Bill and
me, that the writer forgot about nulls. If you really want to not
handle the null with a line of code or two then you ought to add a
comment explicitly SAYING that so the reader won't start distrusting
your code.
> 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.