cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sylvain Lebresne (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-8963) Make OpOrder more intuitive
Date Thu, 16 Jul 2015 10:04:05 GMT

    [ 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)

Mime
View raw message