cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Petrov (Jira)" <j...@apache.org>
Subject [jira] [Commented] (CASSANDRA-15350) Add CAS “uncertainty” and “contention" messages that are currently propagated as a WriteTimeoutException.
Date Fri, 29 Nov 2019 16:06:00 GMT

    [ https://issues.apache.org/jira/browse/CASSANDRA-15350?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16985099#comment-16985099
] 

Alex Petrov commented on CASSANDRA-15350:
-----------------------------------------

 [~yifanc] thank you for the patch. I agree with [~spod] notes on the naming. Maybe other
potential name could be something like {{CASUnknownResultException}}?

Regarding whether you'd like to retry or not. Since contention itself is a result of concurrency,
backing off and not retrying may actually a good idea. In case of uncertainty, I'd say you'd
usually retry with another write rather than reading and then writing, at least that's how
I would imagine application logic in such case. In case of a failed subsequent write, you'll
also get a reason in form of a new value. Similar naming should apply to {{uncertainties}}
in metrics, etc.

Comments: 
  * In {{ErrorMessage#decode}}, you're unconditionally changing the protocol [here|https://github.com/apache/cassandra/compare/trunk...yifan-c:cas-exception-changes#diff-a339339f7a71b38f6a277888f38eb48eR126].
If I understand correctly, we can still have a combination of {{WriteTimeout}} and {{CAS}}
in {{v4}}. So if we receive a message from {{v4}} on a {{v5}} node, we won't be able to interpret
it correctly. You can try and write in-jvm test to test for that, or use in-jvm test facilities
to try and encode message on one version of the node and decode it on the other. 
  * {{ErrorMessage#encode}} we seem to be aided by the fact that {{getBackwardsCompatibleException}}
gives us the right exception. However, since these two parts of code are disjoint, and there's
no guarantee future changes will be careful, I'd add an assertion that [this code path|https://github.com/apache/cassandra/compare/trunk...yifan-c:cas-exception-changes#diff-a339339f7a71b38f6a277888f38eb48eR242]
should be possible only under {{v5}}.
  * Same logic as above applies for size calculation [here|https://github.com/apache/cassandra/compare/trunk...yifan-c:cas-exception-changes#diff-a339339f7a71b38f6a277888f38eb48eR307]
and [here|https://github.com/apache/cassandra/compare/trunk...yifan-c:cas-exception-changes#diff-a339339f7a71b38f6a277888f38eb48eR325].

  * {{CAS_UNCERTAINTY}} clause is new in protocol {{v5}}. We should not attempt to interpret
or write it for the older protocol versions. In fact, for older protocol version we should
still encode it as write timeout to be fully compatible.
  * I'd argue that "uncertainties" should not be counted towards timeouts like [here|https://github.com/apache/cassandra/compare/trunk...yifan-c:cas-exception-changes#diff-71f06c193f5b5e270cf8ac695164f43aR295].
But someone with better operational insight might be a better person to answer this.
  * Unless you're submitting patches to 2.2, 3.0, and 3.11, let's roll back changes to {{IMessageFilters}},
since their API has to be binary compatible with older versions.
  * Should we add timeout tests for responses as well as requests in {{CasWriteTest}}?
  * Is it possible to try and simplify {{testCasWriteTimeoutDueToContention}}, can we achieve
contention with {{N}} threads?
  * Similar comment for {{testCasWriteUncertainty_ProposalNotReplayed}}: both tests peer quite
a lot into implementation internals. If we can't do this, I'd just leave the tests out from
commit since it might be difficult to maintain them. I've also been able to catch a failure
of {{testCasWriteUncertainty_ProposalNotReplayed}} after several thousand runs. Maybe we can
use fuzzing to reproduce both scenarios?
  * Can we add a cross-version ser-de test, too?

Some nits: 
  * In {{ErrorMessage#decode}}, there are extra brackets around {{WRITE_TIMEOUT}} clause.
You can remove those and fix indentation. Same happens in {{CAS_UNCERTAINTY}} case. 
  * If we add comments for {{activate}} and {{deactivate}} for off/on, maybe it's worth to
call those off/on?
  * Maybe it's worth to rename {{Filter}} to {{Sink}} and separate filters from interceptors,
allowing a common class "sink" between those?
  * We don't really need to rename {{equals}} to {{equals0}} 


> Add CAS “uncertainty” and “contention" messages that are currently propagated as
a WriteTimeoutException.
> ---------------------------------------------------------------------------------------------------------
>
>                 Key: CASSANDRA-15350
>                 URL: https://issues.apache.org/jira/browse/CASSANDRA-15350
>             Project: Cassandra
>          Issue Type: Improvement
>          Components: Feature/Lightweight Transactions
>            Reporter: Alex Petrov
>            Assignee: Yifan Cai
>            Priority: Normal
>              Labels: protocolv5, pull-request-available
>         Attachments: Utf8StringEncodeBench.java
>
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Right now, CAS uncertainty introduced in https://issues.apache.org/jira/browse/CASSANDRA-6013
is propagating as WriteTimeout. One of this conditions it manifests is when there’s at least
one acceptor that has accepted the value, which means that this value _may_ still get accepted
during the later round, despite the proposer failure. Similar problem happens with CAS contention,
which is also indistinguishable from the “regular” timeout, even though it is visible
in metrics correctly.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org


Mime
View raw message