[
https://issues.apache.org/jira/browse/CASSANDRA-13747?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16130085#comment-16130085
]
Benedict edited comment on CASSANDRA-13747 at 8/17/17 8:26 AM:
---------------------------------------------------------------
The patch looks good, but while reviewing I got a little suspicious of the
modified line {{DataResolver:479}}, as it seemed that {{n}} and {{x}} were the
wrong way around... and, reading the comment of intent directly above, and
reproducing the calculation, they are indeed.
-Assuming, now correctly defined, that {{n <= x}}, this also obviates the need
for the {{Math.max(x, 1)}} you have introduced. This must be true, given that
we can only have a short-read triggered in the case that we have yielded too
few rows, so we must have fewer than we requested (even if other rows we didn't
know about were introduced by other peers).-
This is _probably_ a significant enough bug that it warrants its own ticket for
record keeping, though I'm fairly agnostic on that decision.
I'm a little concerned about our current short read behaviour, as right now it
seems we should be requesting exactly one row, for any size of under-read,
which could mean extremely poor performance in case of large under-reads.
I would suggest that the outer unconditional {{Math.max}} is a bad idea, has
been (poorly) insulating us from this error, and that we should first be
asserting that the calculation yields a value {{>= 0}} before setting to 1.
was (Author: benedict):
The patch looks good, but while reviewing I got a little suspicious of the
modified line {{DataResolver:479}}, as it seemed that {{n}} and {{x}} were the
wrong way around... and, reading the comment of intent directly above, and
reproducing the calculation, they are indeed.
Assuming, now correctly defined, that {{n <= x}}, this also obviates the need
for the {{Math.max(x, 1)}} you have introduced. This must be true, given that
we can only have a short-read triggered in the case that we have yielded too
few rows, so we must have fewer than we requested (even if other rows we didn't
know about were introduced by other peers).
This is _probably_ a significant enough bug that it warrants its own ticket for
record keeping, though I'm fairly agnostic on that decision.
I'm a little concerned about our current short read behaviour, as right now it
seems we should be requesting exactly one row, for any size of under-read,
which could mean extremely poor performance in case of large under-reads.
I would suggest that the outer unconditional {{Math.max}} is a bad idea, has
been (poorly) insulating us from this error, and that we should first be
asserting that the calculation yields a value {{>= 0}} before setting to 1.
> Fix short read protection
> -------------------------
>
> Key: CASSANDRA-13747
> URL: https://issues.apache.org/jira/browse/CASSANDRA-13747
> Project: Cassandra
> Issue Type: Bug
> Components: Coordination
> Reporter: Aleksey Yeschenko
> Assignee: Aleksey Yeschenko
> Fix For: 3.0.x, 3.11.x
>
>
> {{ShortReadRowProtection.moreContents()}} expects that by the time we get to
> that method, the global post-reconciliation counter was already applied to
> the current partition. However, sometimes it won’t get applied, and the
> global counter continues counting with {{rowInCurrentPartition}} value not
> reset from previous partition, which in the most obvious case would trigger
> the assertion we are observing - {{assert
> !postReconciliationCounter.isDoneForPartition();}}. In other cases it’s
> possible because of this lack of reset to query a node for too few extra
> rows, causing unnecessary SRP data requests.
> Why is the counter not always applied to the current partition?
> The merged {{PartitionIterator}} returned from {{DataResolver.resolve()}} has
> two transformations applied to it, in the following order:
> {{Filter}} - to purge non-live data from partitions, and to discard empty
> partitions altogether (except for Thrift)
> {{Counter}}, to count and stop iteration
> Problem is, {{Filter}} ’s {{applyToPartition()}} code that discards empty
> partitions ({{closeIfEmpty()}} method) would sometimes consume the iterator,
> triggering short read protection *before* {{Counter}} ’s
> {{applyToPartition()}} gets called and resets its {{rowInCurrentPartition}}
> sub-counter.
> We should not be consuming iterators until all transformations are applied to
> them. For transformations it means that they cannot consume iterators unless
> they are the last transformation on the stack.
> The linked branch fixes the problem by splitting {{Filter}} into two
> transformations. The original - {{Filter}} - that does filtering within
> partitions - and a separate {{EmptyPartitionsDiscarder}}, that discards empty
> partitions from {{PartitionIterators}}. Thus {{DataResolve.resolve()}}, when
> constructing its {{PartitionIterator}}, now does merge first, then applies
> {{Filter}}, then {{Counter}}, and only then, as its last (third)
> transformation - the {{EmptyPartitionsDiscarder}}. Being the last one
> applied, it’s legal for it to consume the iterator, and triggering
> {{moreContents()}} is now no longer a problem.
> Fixes: [3.0|https://github.com/iamaleksey/cassandra/commits/13747-3.0],
> [3.11|https://github.com/iamaleksey/cassandra/commits/13747-3.11],
> [4.0|https://github.com/iamaleksey/cassandra/commits/13747-4.0]. dtest
> [here|https://github.com/iamaleksey/cassandra-dtest/commits/13747].
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org
For additional commands, e-mail: commits-h...@cassandra.apache.org
