[
https://issues.apache.org/jira/browse/CASSANDRA-8963?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14629517#comment-14629517
]
Sylvain Lebresne commented on CASSANDRA-8963:
---------------------------------------------
Without getting into the naming debate above, I'd want to say that imo the
current {{OpOrder}} implementation/API is largely ok and I'm not a huge fan of
changing for changing.
Based on my own experience with that class (which thus could be off for other
people), my suspicion is that the 2 main things that makes {{OpOrder}} hard to
approach initially are that:
# the class javadoc does a poor job at explaining clearly/simply what the class
is about. The discussions around 'consumer and producer processing contiguous
batches' and other 'position the work is at' are, I think, too abstract. And
while the example could help, it references methods that are either
non-existent ('seal()') or barely used in the codebase ('isAfter()'), making it
less than ideal for getting an initial grasps of the class and its usage in the
code.
# the actual usages of the class are barely documented. I suspect that's the
biggest problem btw. If you look at the {{readOrdering.start()}} we do at the
beginning of a read, you have no clue why we're doing it. You then scroll to
the definition of {{readOrdering}}, but that give you no real clue on what this
is about because the small comment there is both pretty vague and outdated
({{readOrdering}} is now used for much more than protecting off-heap storage).
Your third inclination is then to go to the {{OpOrder}} class javadoc, hoping
the understanding of the class will bring you enough enlightenment to figure
out why {{readOrdering}} exists on your own. But then the first point kicks in,
at which point you'll probably give up, considering all this unintuitive.
So I would submit that just fixing those 2 things would be good enough. Now,
for the sake of not just criticising without offering anything, I could propose
as a starting point for the class javadoc something like:
{noformat}
/**
* A synchronization primitive that allows a specific operation X to wait until
all other operations started before X are completed.
*
* There is 2 sides to using this class. Given a particular {@code OpOrder
order}:
* 1) each operation for which we might want to wait the completion of should
call {@code order.start()} when they start, which assign them a group
* ({@code OpOrder.Group}), and should close that group once completed
(it's crucial for any group assigned through {@code start()} be closed,
* so an {@code OpOrder.Group} should preferably be protected by a
try-with-ressources).
* 2) an operation that wants to wait on the completion of operations
protected though 1) should issue a barrier {@code order.newBarrier()}, issue it
* {@code order.issue()} and then wait on that barrier ({@code
OpOrder.Barrier.await()}). The guarantee is then that {@code await()} will only
return
* once all the operations protected through 1) that started before the
barrier has been issued have completed (where completed means "have closed
* their assigned group object").
*
* In practice, an example use case of this is the {{readOrdering}} of
{{ColumnFamilyStore}}. It is used to protect every read operation, from
* before we start reading the memtables and sstables, to when the result is
fully read. Which then allows us to protect other operation from
* racing with reads. For instance, to drop a table, we first mark the table
dropped (guaranteeing no new read on that table can start), but
* then issue a new barrier on {{readOrdering}} and wait on it before
discarding the memtables/sstables. This guarantees us that any read that
* may be in-flight when we mark the table dropped is completed before we
delete the memtables/sstables.
*/
{noformat}
Then I'd also add a bigger comment on {{readOrdering}} declaration that sums
all the reasons it is used for, and would do the same for all other OpOrder.
API-wise, the only change I'd maybe suggest is around {{issue()}}. Ideally, I'd
remove {{issue()}} altogether, making the creation of a barrier issuing it
implicitly, because that's a bit simpler. There is only one case where we don't
issue right away and maybe we can slightly refactor the code to not need this?
But if that's not possible (or too contrived), then I'd make {{newBarrier()}}
issue implicitly, remove the references to {{issue()}} in the javadoc above,
and add a {{newUnissuedBarrier()}} method for the rare cases where we need it.
If the class is easier to apprehend then it's ok if you have to go read the
fine-prints for more advanced usages.
Anyway, even if you hate the suggestions above and want to debate naming to
death (which is cool, I'm all for naming death-match), I'd humbly suggest
delaying it post-3.0 because I really think this can wait.
> 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)
