You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "Andres de la Peña (Jira)" <ji...@apache.org> on 2020/06/17 11:32:00 UTC
[jira] [Commented] (CASSANDRA-15459) Short read protection doesn't
work on group-by queries
[ https://issues.apache.org/jira/browse/CASSANDRA-15459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17138356#comment-17138356 ]
Andres de la Peña commented on CASSANDRA-15459:
-----------------------------------------------
I think that the way "row counted" is used to calculate limits is not the cause of the problem here. The comments about this usage in [{{ShortReadPartitionsProtection#counted}}|https://github.com/apache/cassandra/blob/4f50a6712ada5c4298ec860836015ea15049cbda/src/java/org/apache/cassandra/service/DataResolver.java#L762-L768] and [{{ShortReadPartitionsProtection.ShortReadRowsProtection#countedInCurrentPartition}}|https://github.com/apache/cassandra/blob/4f50a6712ada5c4298ec860836015ea15049cbda/src/java/org/apache/cassandra/service/DataResolver.java#L907-L913] do not seem to describe what the method is actually doing. These comments where added during CASSANDRA-10707, replacing the original comments added during CASSANDRA-13794 ([here|https://github.com/apache/cassandra/commit/2bae4ca907ac4d2ab53c899e5cf5c9e4de631f52]). I'm restoring the original description of those methods.
It seems to me that the cause of the error is [here|https://github.com/apache/cassandra/blob/4f50a6712ada5c4298ec860836015ea15049cbda/src/java/org/apache/cassandra/db/filter/DataLimits.java#L966]. Implementations of {{DataLimit.Counter}} are meant to both count results and also to limit them, being a {{StoppingTransformation}}. The method {{DataLimit.Counter#onlyCount}} allows to disable their result-limiting behaviour, so they only count results without transforming them. The counter [{{singleResultCounter}}|https://github.com/apache/cassandra/blob/4f50a6712ada5c4298ec860836015ea15049cbda/src/java/org/apache/cassandra/service/DataResolver.java#L630-L631] in short read protection uses this read-only behaviour, so it counts the replica results without truncating them, in case more replica results are needed after reconciliation. However, the method {{GroupByAwareCounter#applyToRow}} unconditionally returns a {{null}} row in case the read partition has more rows than the specified by the limit, which can violate the count-only behaviour. Something similar happens in {{GroupByAwareCounter#applyToStatic}}. The proposed PR simply takes into account the {{Counter.enforceLimits}} to prevent this filtering.
The dtest PR just adds the excellent test provided by [~jasonstack] in the description, with a minimal change to disable read-repair in 3.11 with Byteman, because we don't have the {{NONE}} repair strategy available in that version. I'm also excluding 3.0 because {{GROUP BY}} was added in 3.10.
CI results:
||branch||ci-cassandra utest||ci-cassandra dtest||CircleCI j8||CircleCI j11||
|[3.11|https://github.com/apache/cassandra/pull/635]|[126|https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-test/126/]|[163|https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-dtest/163/]|[link|https://app.circleci.com/pipelines/github/adelapena/cassandra/53/workflows/ce4d2cad-a811-43af-a215-b4ea71260d0e]||
|[trunk|https://github.com/apache/cassandra/pull/636]|[127|https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-test/127/]|[164|https://ci-cassandra.apache.org/view/all/job/Cassandra-devbranch-dtest/163/]|[link|https://app.circleci.com/pipelines/github/adelapena/cassandra/55/workflows/9f73dadc-a963-43b6-8792-ea5e0c0e17c8]|[link|https://app.circleci.com/pipelines/github/adelapena/cassandra/55/workflows/56a025b0-4c18-4eae-9f77-2c261b1d2cc5]|
CC [~blerer]
> Short read protection doesn't work on group-by queries
> ------------------------------------------------------
>
> Key: CASSANDRA-15459
> URL: https://issues.apache.org/jira/browse/CASSANDRA-15459
> Project: Cassandra
> Issue Type: Bug
> Components: Legacy/Coordination
> Reporter: ZhaoYang
> Assignee: Andres de la Peña
> Priority: Normal
> Labels: correctness
> Fix For: 3.11.7, 4.0-beta
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> [DTest to reproduce|https://github.com/apache/cassandra-dtest/compare/master...jasonstack:srp_group_by_trunk?expand=1]: it affects all versions..
> {code}
> In a two-node cluster with RF = 2
> Execute only on Node1:
> * Insert pk=1 and ck=1 with timestamp 9
> * Delete pk=0 and ck=0 with timestamp 10
> * Insert pk=2 and ck=2 with timestamp 9
> Execute only on Node2:
> * Delete pk=1 and ck=1 with timestamp 10
> * Insert pk=0 and ck=0 with timestamp 9
> * Delete pk=2 and ck=2 with timestamp 10
> Query: "SELECT pk, c FROM %s GROUP BY pk LIMIT 1"
> * Expect no live data, but got [0, 0]
> {code}
> Note: for group-by queries, SRP should use "group counted" to calculate limits used for SRP query, rather than "row counted".
--
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