jmckenzie-dev commented on code in PR #4727:
URL: https://github.com/apache/cassandra/pull/4727#discussion_r3080554199
##########
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:
Historically we've kind of erred on the side of cleaning things up when we
also have another change we're doing in the same domain. "Leave the campsite
cleaner than when you got there" kind of sentiment. Doing it with this patch
also prevents the operational overhead of needing to create a 2nd ticket and
get a 2nd committer +1 and merge.
If it's primarily stylistic and cosmetic (i.e. not refactoring structure,
adding new unrelated classes or interfaces, etc) - I think it's fine either
way. Whichever is easier for you.
And +1 to doing it on all branches or none for sure; want to keep things
consistent since we do merge commits between branches.
--
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]
