sedulam commented on code in PR #4727:
URL: https://github.com/apache/cassandra/pull/4727#discussion_r3075612058
##########
src/java/org/apache/cassandra/db/ReadCommand.java:
##########
@@ -577,57 +579,70 @@ protected void attachTo(BaseRows rows)
this.rows = rows;
}
+ @Override
protected UnfilteredRowIterator applyToPartition(UnfilteredRowIterator
partition)
{
- if (maybeAbort())
- {
- partition.close();
- return null;
- }
-
+ maybeCancel();
return Transformation.apply(partition, this);
}
+ @Override
protected Row applyToRow(Row row)
{
- if (TEST_ITERATION_DELAY_MILLIS > 0)
- maybeDelayForTesting();
-
- return maybeAbort() ? null : row;
+ maybeCancel();
+ return row;
}
- private boolean maybeAbort()
+ private void maybeCancel()
{
- /**
- * TODO: this is not a great way to abort early; why not expressly
limit checks to 10ms intervals?
+ /*
* The value returned by approxTime.now() is updated only every
- * {@link
org.apache.cassandra.utils.MonotonicClock.SampledClock.CHECK_INTERVAL_MS}, by
default 2 millis. Since MonitorableImpl
- * relies on approxTime, we don't need to check unless the
approximate time has elapsed.
+ * {@link
org.apache.cassandra.utils.MonotonicClock.SampledClock.CHECK_INTERVAL_MS}, by
default 2 millis.
+ * Since MonitorableImpl relies on approxTime, we don't need to
check unless the approximate time has elapsed.
*/
- if (lastChecked == approxTime.now())
- return false;
-
- lastChecked = approxTime.now();
+ if (lastCheckedAt == approxTime.now())
+ return;
+ lastCheckedAt = approxTime.now();
if (isAborted())
{
stop();
- return true;
+ throw new QueryCancelledException(ReadCommand.this);
}
+ }
+ }
- return false;
+ private UnfilteredPartitionIterator
withQueryCancellation(UnfilteredPartitionIterator iter)
+ {
+ return Transformation.apply(iter, new QueryCancellationChecker());
+ }
+
+ /**
+ * A transformation used for simulating slow queries by tests.
+ */
+ private static class DelayInjector extends
Transformation<UnfilteredRowIterator>
Review Comment:
This pattern (no `@VisibleForTesting` on `DelayInjector`) comes from 5.0
as-is. I saw these and some other issues, but I thought it might be better to
address them in a separate ticket ([hence this message on
Slack](https://the-asf.slack.com/archives/CK23JSY2K/p1775914971299049)), and
stick to a faithful backport as much as possible.
However, I'm happy to start fixing on this ticket already. Can I do the same
on 5.0/trunk as well? It would be be good to keep the code consistent between
versions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
