Bruce is correct that my refactored code is wrong.
I thought this method was called on the server but it is actually only
called on the client when the server pushes a message to the client through
the subscription queue.
So the current code is correct; just a bit hard to understand.
On Mon, Apr 17, 2017 at 10:17 AM, Bruce Schuchardt <bschucha...@pivotal.io>
wrote:
> +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_E
>>> NABLED_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 <kl...@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}));
>>> }
>>> }
>>>
>>>
>
