virajjasani commented on code in PR #1936:
URL: https://github.com/apache/phoenix/pull/1936#discussion_r1706068243
##########
phoenix-core-server/src/main/java/org/apache/phoenix/iterate/NonAggregateRegionScannerFactory.java:
##########
@@ -465,6 +475,10 @@ public boolean next(List<Cell> results) throws IOException
{
}
}
tuple = nextTuple;
+ if (regionScannerContext != null) {
+ ScannerContextUtil.updateMetrics(regionScannerContext,
scannerContext);
+ regionScannerContext = null;
+ }
Review Comment:
I wonder if it is still possible to not pass the ScannerContext to iterators
above and still use it here?
Probably we can, since we are not really updating any context in
`OrderedResultIterator` and `OffsetResultIterator`? Or i am missing something?
##########
phoenix-core-client/pom.xml:
##########
@@ -438,5 +438,9 @@
<groupId>org.hdrhistogram</groupId>
<artifactId>HdrHistogram</artifactId>
</dependency>
+ <dependency>
Review Comment:
Having phoenix-core-client depend on hbase-server is really bad practice
because it ultimately kills the whole purpose of having two separate
phoenix-core modules, one for client and one of server.
But the bigger problem here is that we use the same iterator class at both
client and server side, and depending on which side uses it, the context like
this can be used.
It would have been much simpler if we had iterators of type `ResultIterator`
only being used by clients and scanners of type `BaseRegionScanner` only being
used by server. Then we could make `ScannerContext` available to only scanners
of type `BaseRegionScanner`.
--
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]
