[
https://issues.apache.org/jira/browse/CASSANDRA-8963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14628611#comment-14628611
]
Benedict commented on CASSANDRA-8963:
-------------------------------------
Well, that's disappointing. I'd hoped this would be easy, but it looks like
we're going to disagree quite extensively!
As far as naming is concerned, I'm the opposite end of the spectrum. Extraneous
duplication of information is just unpleasant.
* {{CloseableOp}}? {{OrderedOp}} (or {{PartiallyOrderedOp}}), I could get
behind, but {{Closeable}} offers no more information than inherent to the
class, nor information about what it _delivers_
* Reusable is a misnomer, since it implies you should keep it hanging around.
Perhaps {{CheckpointingOp}}? The idea is to use this for long running
operations that could be split into multiple operations, but to avoid
starting/stopping operations all of the time, instead calling the {{restart}}
method (which we could instead call {{checkpoint}})
* {{OpGroup}} doesn't really convey that it is ordered wrt the others.
{{OpPhase}}?
* {{OpGroupBarrier}}: {{OpPhaseBoundary}}?
* {{LinkedOpGroupList}} this I realy hate, sorry. Leaks way too many
implementation details, IMO _confusing_ what it's really delivering, rather
than clarifying. How about {{OpOrder}} :) or more correctly, {{OpPartialOrder}}
* {{startNewOp}}, {{tryRegister}}: sure, I won't quibble, so we can find some
easy wins of common ground...
bq. On the whole, the ratio of comments to actual code in Order and Phase gives
me the immediate impressions
This is something I'm really going to have to disagree with strongly. The
majority of information in {{Order}} is just to make certain that the user has
clear API definition and examples, and is much less verbose than JDK
equivalents. It may be that I've done a bad job of that, but it cannot possibly
be a good idea to simply get rid of it (rather than improve it). {{Phase}},
however, has two main bodies of comments, and they're both around ensuring the
reader has all of the context for any subtlety of implementation. Much of these
comments have existed in the codebase all along, and I'm generally campaigning
for _more_ comments in the codebase, not fewer. Especially in places where it
isn't immediately obvious why certain decisions were made, or what interactions
the code has with other actors that a new (or returning) reader may not
immediately consider. Concurrent algorithms definitely fit this bill (that
said, lines 34-38 and 50-51 in Phase are stale, misdirecting, and can be
removed completely. I wasn't actually expecting a full review of the internals;
just a high-level API review...).
bq. I think it would be a good idea to refactor out the concept of a
thread-safe BitSet implementation to its own class
It's only a counter and two flag bits.... However the stale comments in lines
50-51 may confuse that. I don't think splitting it out would simplify, but I
can certainly create some static methods to make the modifications to this more
declarative in the methods that update it.
*Even though (or especially because) this looks like it may be tortuous for us
both, I appreciate your taking the time to review.*
I'm genuinely thrown a bit by your statement about comments, though. We've had
a lot of problems in the codebase from subtlety not being clearly (and perhaps
repeatedly, near all points of effect/concern) stated for future authors. While
I agree the difficulty of understanding OpOrder has been overstated, it is
still clearly not a trivial problem, and a paragraph in the two main functional
classes loosely outlining the states they can take, plus a couple of short
paragraphs in the {{unlink}} method where the riskiest (and most complex)
execution overlaps occur, seem exactly what comments were born for. I'm at a
loss as to how we're at such wildly different ends of the spectrum on this.
> 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)
