[
https://issues.apache.org/jira/browse/CASSANDRA-8963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14629907#comment-14629907
]
Joshua McKenzie commented on CASSANDRA-8963:
--------------------------------------------
bq. MultiPhaseOp is another possibility, to indicate it crosses phase
boundaries.
Works for me.
bq. For this reason I'm not a fan of OpPhaseChain either. I would prefer
something like OpPhaser, or something that conveys that it is a mechanism for
creating a partial or phased ordering of operations.
I don't love OpPhaser but neither do I hate it. That'll also work for me.
Lastly, regarding the commenting style - it's just that: style. Breaking
methods up into blocks of operations w/comments explaining what they're doing
is fine. I prefer to have comments serving as a warning that something tricky
is going on and the code clearly explaining the rest, but it's just a
preference.
One thing I've been stewing on since yesterday - first off, the uncommented
bit-magic in here is clearly not the easiest to digest. That being said, the
OpPOrder.close() implementation currently on 2.2:
{code:title=current}
public void close()
{
while (true)
{
int current = running;
if (current < 0)
{
if (runningUpdater.compareAndSet(this, current, current + 1))
{
if (current + 1 == FINISHED)
{
// if we're now finished, unlink ourselves
unlink();
}
return;
}
}
else if (runningUpdater.compareAndSet(this, current, current - 1))
{
return;
}
}
}
{code}
Phase.closeOne():
{code:title=current branch}
void closeOne()
{
assert next == null || next.prev == this;
while (true)
{
int current = state;
int next = (current & RUNNING) - 1;
next |= (current & ~RUNNING);
if (stateUpdater.compareAndSet(this, current, next))
{
if (isFinished(next))
unlink();
return;
}
}
}
{code}
I feel like we're moving in a less readable direction here; is there a reason
an AtomicInteger wouldn't do for our purposes?
Lastly, Sylvain's advice seems sound to me. Having a header comment of that
sort would go a long way to setting the stage to understand this code structure
more for new users of the construct.
> Make OpOrder more intuitive
> ---------------------------
>
> Key: CASSANDRA-8963
> URL: https://issues.apache.org/jira/browse/CASSANDRA-8963
> Project: Cassandra
> Issue Type: Improvement
> Components: Core
> Reporter: Benedict
> Assignee: Benedict
> Priority: Minor
> Fix For: 3.x
>
>
> There has been plenty of feedback about OpOrder being unintuitive. As well as
> revisiting the naming, I propose to introduce an Action object with RAII
> (AutoCloseable) protection that should be more obvious to users of the API.
> We can also then protect this by a Ref instance for use cases where the
> action lifetime is illdefined, and perhaps also introduce some checks for
> actions whose lifetimes extend beyond a sensible limit to report those where
> the object reference is retained.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
