You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "maedhroz (via GitHub)" <gi...@apache.org> on 2024/02/22 21:10:07 UTC
[PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
maedhroz opened a new pull request, #3122:
URL: https://github.com/apache/cassandra/pull/3122
(no comment)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1502969211
##########
src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java:
##########
@@ -370,13 +372,20 @@ private UnfilteredRowIterator queryStorageAndFilter(PrimaryKey key)
return null;
lastKey = key;
+ long startTimeNanos = Clock.Global.nanoTime();
try (UnfilteredRowIterator partition = queryController.queryStorage(key, executionController))
{
queryContext.partitionsRead++;
queryContext.checkpoint();
- return applyIndexFilter(key, partition, filterTree, queryContext);
+ UnfilteredRowIterator filtered = applyIndexFilter(key, partition, filterTree, queryContext);
+
+ // Note that we record the duration of the read after post-filtering, which actually
+ // materializes the rows from disk.
+ queryController.tableQueryMetrics.postFilteringReadLatency.update(Clock.Global.nanoTime() - startTimeNanos, TimeUnit.NANOSECONDS);
Review Comment:
I went back and forth on this a little bit. Especially since I'd like to batch row reads into the same partition read sooner rather than later, and even when we read single rows, we're issuing a partition read, it felt unnecessary to qualify the already long name even further. WDYT?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "mike-tr-adamson (via GitHub)" <gi...@apache.org>.
mike-tr-adamson commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1502573302
##########
src/java/org/apache/cassandra/index/sai/plan/QueryController.java:
##########
@@ -73,7 +73,7 @@ public class QueryController
private final ColumnFamilyStore cfs;
private final ReadCommand command;
private final QueryContext queryContext;
- private final TableQueryMetrics tableQueryMetrics;
+ public final TableQueryMetrics tableQueryMetrics;
Review Comment:
This is not a big deal but it seems that we could avoid this change if we stored the `tableQueryMetrics` in the `StorageAttachedIndexSearcher` and make `ResultRetriever` an inner class rather than a static inner class.
If we choose not to do that, can the `tableQueryMetrics`, at least, be separated, so that the `public` and `private` members are separate.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "mike-tr-adamson (via GitHub)" <gi...@apache.org>.
mike-tr-adamson commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1502578904
##########
src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java:
##########
@@ -370,13 +372,20 @@ private UnfilteredRowIterator queryStorageAndFilter(PrimaryKey key)
return null;
lastKey = key;
+ long startTimeNanos = Clock.Global.nanoTime();
try (UnfilteredRowIterator partition = queryController.queryStorage(key, executionController))
{
queryContext.partitionsRead++;
queryContext.checkpoint();
- return applyIndexFilter(key, partition, filterTree, queryContext);
+ UnfilteredRowIterator filtered = applyIndexFilter(key, partition, filterTree, queryContext);
+
+ // Note that we record the duration of the read after post-filtering, which actually
+ // materializes the rows from disk.
+ queryController.tableQueryMetrics.postFilteringReadLatency.update(Clock.Global.nanoTime() - startTimeNanos, TimeUnit.NANOSECONDS);
Review Comment:
Just to clarify, this is recording the per-partition post-filtering latency. Do we want to change the metric name to reflect this. Something like `postFilteringPartitionReadLatency`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz closed pull request #3122: CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage
URL: https://github.com/apache/cassandra/pull/3122
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#issuecomment-1965334169
Committed as https://github.com/apache/cassandra/commit/1d7bae3697b97e64de2c2b958427ef86a1b17731
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "maedhroz (via GitHub)" <gi...@apache.org>.
maedhroz commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1502978740
##########
src/java/org/apache/cassandra/index/sai/plan/QueryController.java:
##########
@@ -73,7 +73,7 @@ public class QueryController
private final ColumnFamilyStore cfs;
private final ReadCommand command;
private final QueryContext queryContext;
- private final TableQueryMetrics tableQueryMetrics;
+ public final TableQueryMetrics tableQueryMetrics;
Review Comment:
It feels like we should have done that anyway. `QueryController` is an odd place for this to be when `ResultRetriever` is the thing that actually actuates recording the metrics...
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "mike-tr-adamson (via GitHub)" <gi...@apache.org>.
mike-tr-adamson commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1502578904
##########
src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java:
##########
@@ -370,13 +372,20 @@ private UnfilteredRowIterator queryStorageAndFilter(PrimaryKey key)
return null;
lastKey = key;
+ long startTimeNanos = Clock.Global.nanoTime();
try (UnfilteredRowIterator partition = queryController.queryStorage(key, executionController))
{
queryContext.partitionsRead++;
queryContext.checkpoint();
- return applyIndexFilter(key, partition, filterTree, queryContext);
+ UnfilteredRowIterator filtered = applyIndexFilter(key, partition, filterTree, queryContext);
+
+ // Note that we record the duration of the read after post-filtering, which actually
+ // materializes the rows from disk.
+ queryController.tableQueryMetrics.postFilteringReadLatency.update(Clock.Global.nanoTime() - startTimeNanos, TimeUnit.NANOSECONDS);
Review Comment:
Just to clarify, this is recording the per-partition post-filtering read latency. Do we want to change the metric name to reflect this. Something like `postFilteringPartitionReadLatency`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "mike-tr-adamson (via GitHub)" <gi...@apache.org>.
mike-tr-adamson commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1503132787
##########
src/java/org/apache/cassandra/index/sai/plan/StorageAttachedIndexSearcher.java:
##########
@@ -370,13 +372,20 @@ private UnfilteredRowIterator queryStorageAndFilter(PrimaryKey key)
return null;
lastKey = key;
+ long startTimeNanos = Clock.Global.nanoTime();
try (UnfilteredRowIterator partition = queryController.queryStorage(key, executionController))
{
queryContext.partitionsRead++;
queryContext.checkpoint();
- return applyIndexFilter(key, partition, filterTree, queryContext);
+ UnfilteredRowIterator filtered = applyIndexFilter(key, partition, filterTree, queryContext);
+
+ // Note that we record the duration of the read after post-filtering, which actually
+ // materializes the rows from disk.
+ queryController.tableQueryMetrics.postFilteringReadLatency.update(Clock.Global.nanoTime() - startTimeNanos, TimeUnit.NANOSECONDS);
Review Comment:
I'm good with that. We only read partitions right now so it would be excessive.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-18940 Record latencies for SAI post-filtering reads against local storage [cassandra]
Posted by "mike-tr-adamson (via GitHub)" <gi...@apache.org>.
mike-tr-adamson commented on code in PR #3122:
URL: https://github.com/apache/cassandra/pull/3122#discussion_r1503132091
##########
src/java/org/apache/cassandra/index/sai/plan/QueryController.java:
##########
@@ -73,7 +73,7 @@ public class QueryController
private final ColumnFamilyStore cfs;
private final ReadCommand command;
private final QueryContext queryContext;
- private final TableQueryMetrics tableQueryMetrics;
+ public final TableQueryMetrics tableQueryMetrics;
Review Comment:
When you look at the change for it, it looks so much better and simplifies the code again.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org