[
https://issues.apache.org/jira/browse/CASSANDRA-18086?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17643558#comment-17643558
]
Josh McKenzie commented on CASSANDRA-18086:
-------------------------------------------
The patch LGTM. I have some slight concerns with {{ContentionStrategy.java}} as
I read through it to convince myself this patch is correct; there's little to
no documentation about the expectation of units being passed into various
functions nor annotation around the unit being passed back out. I also have the
luxury of having another implementation of this to compare the upstreamed to;
the original did indeed have a slightly different approach to calculating wait
that made it clear the intent there:
{code:java}
long wait =
MICROSECONDS.toNanos(ThreadLocalRandom.current().nextLong(minWaitMicros,
maxWaitMicros)); {code}
In this case, while simple inspection of the code after the fact of observing
the regression shows probable unit mismatch:
{code:java}
long minWaitMicros = min.get(attempts);
long maxWaitMicros = max.get(attempts);
long minDeltaMicros = minDelta.get(attempts);
if (minWaitMicros + minDeltaMicros > maxWaitMicros)
{
maxWaitMicros = minWaitMicros + minDeltaMicros;
if (maxWaitMicros > this.max.max)
{
maxWaitMicros = this.max.max;
minWaitMicros = max(this.min.min, min(this.min.max,
maxWaitMicros - minDeltaMicros));
}
}
// REVIEW EDIT: micros go in, have to assume micros come back out?
long wait = waitRandomizer.wait(minWaitMicros, maxWaitMicros, attempts);
return nanoTime() + wait;
}
{code}
My broad concern is that the lack of annotation about expected units of time
for any of the {{WaitInterface}} implementers in the class as well as other
supporting methods leave ambiguity that _theoretically_ encourages this class
of error.
Now, that said, many (most? all?) of these supporting functions are actually
time unit agnostic so could be used for both nano and microsecond precision
units, so there's an argument here to not clutter the interface and
implementation of things w/names or comments that bind us to a certain unit of
operation. If we back out to the primary user of this in {{Paxos.cas()}} we can
see that the primary proposeDeadline and commitDeadline are in nanos and
extrapolate.
So that's all editorial concern about the lack of annotation in the class, well
outside the scope of the change here to unblock 4.1 GA. So I'm +1 on the
correctness of what's here but would love a follow-on conversation w/someone
more familiar with the cas / Paxos codebase as to whether leaving it unit-free
is adding value on balance.
> Bug fix for 'wait' time to be in nanoseconds for consistency instead of
> microseconds
> ------------------------------------------------------------------------------------
>
> Key: CASSANDRA-18086
> URL: https://issues.apache.org/jira/browse/CASSANDRA-18086
> Project: Cassandra
> Issue Type: Bug
> Components: Feature/Lightweight Transactions
> Reporter: Marianne Lyne Manaog
> Assignee: Marianne Lyne Manaog
> Priority: Normal
> Fix For: 4.1.x, 4.x
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While working on benchmarking Paxos improvements in OSS Cassandra,
> [~benedict] was asked to review the work and thus found that the wait time
> was input in microseconds but then output incorrectly in the
> ContentionStrategy.java file, i.e., by summing up a wait time in nanoseconds
> with another wait time in microseconds (two different units used mistakenly).
> So, Benedict suggested the fix whereby the output wait time was then enforced
> to be consistently in nanoseconds.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
