+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 <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}));
}
}
