sedulam commented on code in PR #4727:
URL: https://github.com/apache/cassandra/pull/4727#discussion_r3076192002
##########
src/java/org/apache/cassandra/db/ReadCommandVerbHandler.java:
##########
@@ -87,17 +88,28 @@ public void doVerb(Message<ReadCommand> message)
{
response = command.createResponse(iterator,
controller.getRepairedDataInfo());
}
+ catch (AssertionError t)
+ {
+ throw new AssertionError(String.format("Caught an error while
trying to process the command: %s", command.toCQLString()), t);
+ }
+ catch (QueryCancelledException e)
+ {
+ logger.debug("Query cancelled (timeout)", e);
+ response = null;
+ assert !command.isCompleted() : "Read marked as completed despite
being aborted by timeout to table " + command.metadata();
Review Comment:
Sounds good! Is there a ticket already to address the rest of the asserts in
the production code?
##########
src/java/org/apache/cassandra/db/ReadCommandVerbHandler.java:
##########
@@ -87,17 +88,28 @@ public void doVerb(Message<ReadCommand> message)
{
response = command.createResponse(iterator,
controller.getRepairedDataInfo());
}
+ catch (AssertionError t)
+ {
+ throw new AssertionError(String.format("Caught an error while
trying to process the command: %s", command.toCQLString()), t);
+ }
+ catch (QueryCancelledException e)
+ {
+ logger.debug("Query cancelled (timeout)", e);
+ response = null;
+ assert !command.isCompleted() : "Read marked as completed despite
being aborted by timeout to table " + command.metadata();
+ }
- if (!command.complete())
+ if (command.complete())
+ {
+ Tracing.trace("Enqueuing response to {}", message.from());
Review Comment:
This overload early-returns if tracing is off. Only formats the string when
tracing is active. No string construction happens when tracing is disabled. I
think it's fine that way, right?
```
public static void trace(String format, Object arg)
{
final TraceState state = instance.get();
if (state == null) // inline isTracing to avoid implicit two calls
to state.get()
return;
state.trace(format, arg);
}
```
##########
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:
I did for 5.0 and trunk as well, but let me know if I should revert those
changes.
--
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]
