I generally agree about handling null, but note that FindBugs will complain
if you check for null and it knows that it can never be null.
Byron Nevins wrote on 9/24/13 8:11 AM:
>
> 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?
>