[
https://issues.apache.org/jira/browse/CASSANDRA-8963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14628544#comment-14628544
]
Joshua McKenzie commented on CASSANDRA-8963:
--------------------------------------------
I'm not going to go too deep into this right now since I'm in the middle of MV
review, but some initial thoughts that can extrapolate out across the library:
* I much prefer code that documents itself via strong, clear naming than by
large blocks of comments.
* For example, a quick renaming example, totally off-the-cuff just to
illustrate the point:
** "Op" to "ClosableOp"
** "MultiOp" to "ReusableClosableOp"
** "Phase" to "OpGroup"
** "Barrier" to "OpGroupBarrier"
** "Order" to "LinkedOpGroupList"
While the names are clearly a lot more verbose, they also immediately hint at
the relationships between the classes as well as their intended usage
(closable, reusable, etc) and underlying data structure. These names I'm
tossing out here are really just quick jots so there may be some serious flaws
in them that I'm missing but I hope it illustrates the point. Also, nesting
"Closable" in the name may help to reinforce that they're expected to be used
with the try-with-resource idiom for example.
In general, I think some method renaming would also go a long way to clarifying
API intent. For instance, renaming {{Order.start}} (start what? The Order? What
order?) to {{LinkedOpGroupList.startNewOp()}}, Order.startMulti (Start multi
what? Multi order?) to {{LinkedOpGroupList.startReusableOp}},
{{phase.register}} to {{Phase.tryRegister}} (though boolean return implies
potential of failure, so this is nitpicky more than anything), etc. All very
trivial changes that may help smooth over the initial hump of absorbing the
intent of a tricky bit of code.
On the whole, the ratio of comments to actual code in Order and Phase gives me
the immediate impressions that 1) some of that should be moved to a design doc
people could reference for an initial overview, 2) some of that should be
encoded into method and variable names to self-document, and 3) what you're
doing here is incredibly complicated and difficult, when in reality it's not
all that bad.
I think it would be a good idea to refactor out the concept of a thread-safe
BitSet implementation to its own class so it doesn't clutter the implementation
here. That strikes me as something we could likely re-use in other contexts and
it's doing Phase no favors regarding readability.
So that's kind of my first pass of thoughts about it - sorry for the delay, and
this ended up not being as shallow as I'd initially expected.
I'll just go ahead and set myself as reviewer here since that kind of just
happened.
> 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)
