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?
>