Hi, Byron:
Cc: Bill:
I have created another patch which take null check into consideration
and attached the patch according to your feedbacks. Please review again
about this new patch.
Thanks
Regards
Jeremy Lv
-----Original Message-----
From: W Byron Nevins [mailto:byron.nevins_at_oracle.com]
Sent: Wednesday, September 25, 2013 2:09 AM
To: Bill Shannon
Cc: lvsongping; dev_at_glassfish.java.net
Subject: Re: [Review request] About GLASSFISH-20696
I believe it can be null.
Sent from my iPhone
On Sep 24, 2013, at 1:50 PM, Bill Shannon <bill.shannon_at_oracle.com> wrote:
> 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?
>