cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Benedict (JIRA)" <>
Subject [jira] [Commented] (CASSANDRA-8963) Make OpOrder more intuitive
Date Wed, 15 Jul 2015 21:39:05 GMT


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...

- // 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
- // 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

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

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:
>             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

View raw message