[
https://issues.apache.org/jira/browse/CASSANDRA-8963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14628794#comment-14628794
]
Benedict commented on CASSANDRA-8963:
-------------------------------------
bq. it's easy to get offended
I'm not at all offended, just reticent to enter into conflict that appeared to
be on the cards, even if it is all good natured. But, in for a penny...
On the naming, I'm not a _fan_, however I'll mostly roll over. To state my
thoughts, though:
* RestartableOp again implies you should leave it dormant, which is actively a
bad idea (I realise restart is what its method is named). It's just about
periodically declaring you're happy to cross an Op/Phase boundary. MultiPhaseOp
is another possibility, to indicate it crosses phase boundaries.
* As to the Op's ordering, it is a _property_ of the Phase, but the Phase is
simply a construct to _provide_ a partial order between operations. From my
POV, names should always elevate _intent_ above _implementation_.
* 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.
** As to encoding characteristics in names, I would typically agree, but here
the relevant nuances are not really tied to its being a linked-list. Its
performance risks are mostly associated with the mutex ownership for barrier
issuance, waiting on barriers, ordered cleanup of phases, and most critically
bounded runtime of any single Op.
bq. I think there's a nice middle-ground between the lack of commenting so
prevalent in the code-base and the giant wall of text that a first glance at
this branch feels like.
I may be a little chatty in my approach to comments sometimes, but I find that
overly terse comments can leave out important details. To give examples, by
rebutting some of your examples...
{quote}
- // prevents any further operations starting against this Phase
one sentence method description (I think not unwarranted here)
- // if there are no running operations, calls unlink; otherwise, we let the
last op to close call it.
second clause is important to relate this to other methods in the class
that interleave concurrently with this, without which behaviour would be
broken; it is contextualised by first clause
- // if we hit an unfinished ancestor, we're not COMPLETE, so leave it to the
ancestor to clean up when done
this second clause (again contextualised by the first) is very important:
the responsibility of the ancestor to ensure cleanup is performed is absolutely
critical to correctness, and so declaring it here when it is not in any way
expressed by the code is absolutely critical
{quote}
I definitely agree there's some redundancy, such as "// our waiters are good to
go, so signal them" but if we had to make a choice, I'd prefer a couple too
many than a couple too few. _Some_ of this may be a matter of style, too, so it
may be worth a brief meta-discussion about this:
bq. Comments that simply restate what the code does are prone to atrophy over
time
I agree, _unless_ they provide structural scaffolding, as I try to make them do
these days (not referencing these old specimens). If comments break a logical
flow up into discrete units (say, 3-10 LOC), so that each unit is easily
corroborated to match the comment, and the whole flow can be understood by
_only_ the comments, I think this makes digestion of the code far far easier.
That all said, I agree these comments are _certainly_ imperfect, having been
written early on in my tenure on a public community project, at which point my
approach to commenting was... unrefined. They were also perhaps overspecified
exactly because there was a degree of revulsion expressed to OpOrder when it
first appeared. Anyway, if _that's_ all we're talking about, we really don't
have as much distance between us as I read into. I'll go and spruce them up.
> 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)
