[
https://issues.apache.org/jira/browse/CASSANDRA-8963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14628692#comment-14628692
]
Joshua McKenzie commented on CASSANDRA-8963:
--------------------------------------------
Let me start this with the TL;DR I should have started my last comment with:
* New structure much better.
* Would like more descriptive names.
* Would like some comments:
** Encoded via naming/structure
** removed
** kept
bq. Well, that's disappointing. I'd hoped this would be easy, but it looks like
we're going to disagree quite extensively!
Nah, don't be disappointed. Starting at far extremes means one, or both, of us
have some good distance to move on compromise on this. It also probably means
there's a miscommunication on this somewhere (addressed below)
bq. 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
The op itself isn't ordered beyond the scope of it being part of a Phase - the
order is a meta-attribute of the "container" the op lives within, correct? I'm
fine with it just being named Op - this is the one I felt least strongly about,
but was more attempting to illustrate a point - that names can (should)
communicate more about a class than "This class is an Operation" if possible.
OrderableOp, PhaseableOp, something that illustrates the coupling/relationship
in the design and the parent/child relationship. I don't foresee us using the
Op outside this structure.
bq. 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)
RestartableOp?
bq. OpGroup doesn't really convey that it is ordered wrt the others. OpPhase?
Hm. It doesn't convey that. I think I prefer OpPhase
bq. OpGroupBarrier: OpPhaseBoundary?
WFM
bq. 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
It does link the implementation details in the name, sure, but modern IDE's
allow us to refactor easily and, to me, it helps to have immediate information
about the structure and performance characteristics of the data structure
without opening it and digging around in the implementation details. I'm
willing to settle on OpPhaseChain (one I toyed with in my previous comment but
decided to go all out instead) - what say you?
bq. it cannot possibly be a good idea to simply get rid of it (rather than
improve it).
I'm not suggesting we get rid of it but rather proposing that, if you feel it's
required to make sense of the code, I'd recommend revisiting your structuring
and naming of the code in question first to see if it's possible to *show* via
the code itself rather than resorting to *telling* via comments. Having methods
and classes that are then decorated with a large block of comments to explain
"what" they're doing rather than "why" they're doing it is violates your first
point about disliking redundancy. An example of a comment block that's
[half-redundant|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Order.java#L80-L81]
and [half-conveying a subtlety of
design|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Order.java#L82-L83]
bq. 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.
I agree and disagree. 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. Again - I think a lot of the comments
should be implicit by the naming and structure of the Classes and methods that
are being used.
bq. I wasn't actually expecting a full review of the internals; just a
high-level API review...).
I wasn't planning on it. I got a bit carried away but implementation details
stuck out to me as I was looking at the higher-level structure.
bq. 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.
At the very least, comment on why the bit twiddling (the need for a thread-safe
BitSet)? Again, comments about "why" and in rare cases (bit-wise ops), the what.
bq. Even though (or especially because) this looks like it may be tortuous for
us both, I appreciate your taking the time to review.
Again, nah. Not torturous. Better we have a lot of different perspectives going
into the code-base than a few. If either/both of us come away from this with an
even slightly changed perspective, it's worth it (assuming that perspective
isn't just agitation with one another ;))
bq. 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.
As I mentioned above, I genuinely don't think there's actualy *that* much
subtlety in and around what you're doing w/OpOrder/OpGroup, but that the
nomenclature, structure, and extensive commenting make it appear quite
different to developers that first step into the structure. Right now, the
immediate impression (at least for me) was that there was a uniform level of
high complexity and subtlety with this code, rather than there being a natural
intuitive understanding of the parts that are simple, and a hot-spot of
comments surrounding parts that are complex/subtle.
bq. 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.
Let me be clear: I'm in no way advocating for getting rid of comments entirely,
or all the comments in this code, or even most of the comments. Or even the
comments you're referring to here. Upon re-reading my earlier comment here on
JIRA (and knowing your stance w/regards to wanting to change our general
culture w/regards to this issue), I should have clarified further. What I'm
looking at here are specific comments that seem extraneous. Some examples (and
I'm not trying to nit-pick, just hopefully clarify what I'm trying to get at):
* [Restating what expire()
does|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Phase.java#L69-L71]
* [Restating what register()
does|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Phase.java#L89]
* [Some more
restating|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Phase.java#L143]
* [More
restating|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Phase.java#L150]
* [An example of a very necessary comment discussing subtleties of
implementation|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Phase.java#L153-L158]
(though, for the sake of completeness, I'd argue the comment on line 153 is
redundant)
* [Something that might be served by encoding the state in the class rather
than
commenting|https://github.com/belliottsmith/bes-utils/blob/master/src/bes/concurrent/order/Phase.java#L23-L30]
In general, I think there's a sweet spot where the code comments itself where
it can, and where we assist it with comments where it cannot or does not based
on the audience. Comments that simply restate what the code does are prone to
atrophy over time as the code changes which are far more of a hindrance than a
help. Also, I don't believe that adding a large number of comments makes up for
having a class named "Order", which in no way really communicates what it is.
An order of... what? Ordered with respect to? Is it a noun or verb?
Finally, I'd like to acknowledge that it's difficult to really hone in on
things like this as it skirts close to the nebulous realm of "personal taste"
and it's easy to get offended - that's definitely not my intent.
> 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)
