geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Schuchardt <bschucha...@pivotal.io>
Subject Re: Conditional bug in LocalRegion.concurrencyConfigurationCheck
Date Mon, 17 Apr 2017 17:17:05 GMT
+1
I think that code makes the method easier to understand but I don't 
think the current message would be incorrect.  The log message is only 
issued if the server's setting doesn't match the client's setting, so 
!clientCCEnabled is going to equal the serverCCEnabled that you've 
introduced.

Le 4/17/2017 à 9:58 AM, Darrel Schneider a écrit :
> The first part of the condition "!this.concurrencyMessageIssued" is just
> simply to make sure we only log this message once.
> The second part "(tag != null) != this.concurrencyChecksEnabled" could be
> changed to this:
>    boolean serverCCEnabled = this.concurrencyChecksEnabled;
>    boolean clientCCEnabled = tag != null;
>    if (clientCCEnabled != serverCCEnabled)
>      log message
>
> The only bug I see in this method is in the actual log message.
> Arg 0 describes how the server has CC configured and arg 1 describes how
> the client has CC configured.
> But by passing "!this.concurrencyChecksEnabled" as arg 0 the message will
> actually tell you how the server is NOT configured.
> And by passing "this.concurrencyChecksEnabled" as arg 1 the message will
> actually tell you how the client is NOT configured.
>
> If the code was changed to use these two booleans then they could also be
> passed as the args to the log message and I think this method would be
> easier to understand.
>
> The new code would be this:
>
>>    private void concurrencyConfigurationCheck(VersionTag tag) {
>>      if (this.concurrencyMessageIssued) {
>>        return;
>>      }
>>      boolean serverConcurrencyChecksEnabled = this.concurrencyChecksEnabled;
>>      boolean clientConcurrencyChecksEnabled = tag != null;
>>      if (clientConcurrencyChecksEnabled != serverConcurrencyChecksEnabled) {
>>        this.concurrencyMessageIssued = true;
>>        logger.info(LocalizedMessage.create(
>>
>> LocalizedStrings.LocalRegion_SERVER_HAS_CONCURRENCY_CHECKS_ENABLED_0_BUT_CLIENT_HAS_1_FOR_REGION_2,
>>            new Object[] {serverConcurrencyChecksEnabled,
>> clientConcurrencyChecksEnabled, this}));
>>      }
>>    }
> What do you think?
>
>
> On Fri, Apr 14, 2017 at 6:50 PM, Kirk Lund <klund@apache.org> wrote:
>
>> I can't quite make out what the conditional is actually supposed to be
>> checking in the second half but it definitely looks wrong to me. Anyone
>> familiar with this method or what it's supposed to be doing?
>>
>> private void concurrencyConfigurationCheck(VersionTag tag) {
>>    // TODO: double negative in next line must be a bug
>>    if (!this.concurrencyMessageIssued && *((tag != null) !=
>> this.concurrencyChecksEnabled)*) {
>>      this.concurrencyMessageIssued = true;
>>      logger.info(LocalizedMessage.create(
>>
>> LocalizedStrings.LocalRegion_SERVER_HAS_CONCURRENCY_CHECKS_
>> ENABLED_0_BUT_CLIENT_HAS_1_FOR_REGION_2,
>>          new Object[] {!this.concurrencyChecksEnabled,
>> this.concurrencyChecksEnabled, this}));
>>    }
>> }
>>


Mime
View raw message