Please check it in
On 9/26/13 6:43 PM, lvsongping wrote:
> Hi, Byron, Bill
> Cc: dev list
>
> If none of you have any objections or feedbacks about the latest patch,
> I will check in the changes in 10/7.
>
> Thanks
>
> Regards
> Jeremy Lv
>
> -----Original Message-----
> From: Bill Shannon [mailto:bill.shannon_at_oracle.com]
> Sent: Friday, September 27, 2013 4:35 AM
> To: lvsongping
> Cc: byron.nevins_at_oracle.com; dev_at_glassfish.java.net
> Subject: Re: FW: [Review request] About GLASSFISH-20696
>
> 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?
--
---
All Generalizations are false. Including this one.