Looks good to me.
lvsongping wrote on 09/24/2013 08:01 PM:
> 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?
>>