Apache9 commented on a change in pull request #4106:
URL: https://github.com/apache/hbase/pull/4106#discussion_r825776219
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncClientScanner.java
##########
@@ -206,15 +246,34 @@ private void openScanner() {
addListener(timelineConsistentRead(conn.getLocator(), tableName, scan,
scan.getStartRow(),
getLocateType(scan), this::openScanner, rpcTimeoutNs,
getPrimaryTimeoutNs(), retryTimer,
conn.getConnectionMetrics()), (resp, error) -> {
- if (error != null) {
- consumer.onError(error);
- return;
+ final Span localSpan = span;
+ try (Scope ignored = localSpan.makeCurrent()) {
+ if (error != null) {
+ try {
+ consumer.onError(error);
+ return;
+ } finally {
+ TraceUtil.setError(localSpan, error);
+ localSpan.end();
+ }
+ }
+ startScan(resp);
}
- startScan(resp);
});
}
public void start() {
- openScanner();
+ final Span localSpan = new TableOperationSpanBuilder(conn)
Review comment:
The only place where we create this scanner and call start on it is in
RawAsyncTableImpl.scan method, where both these two operations are in the same
thread, so I think here we could make the Span field final and create it in the
constructor, so we do not need to care about concurrency access any more.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncScanSingleRegionRpcRetryingCaller.java
##########
@@ -573,7 +574,8 @@ private void call() {
resetController(controller, callTimeoutNs, priority);
ScanRequest req = RequestConverter.buildScanRequest(scannerId,
scan.getCaching(), false,
nextCallSeq, scan.isScanMetricsEnabled(), false, scan.getLimit());
- stub.scan(controller, req, resp -> onComplete(controller, resp));
+ Context context = Context.current();
Review comment:
What is the purpose here? Wrap the callback in the span in the current
context?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableImpl.java
##########
@@ -43,6 +46,7 @@
*/
@InterfaceAudience.Private
class AsyncTableImpl implements AsyncTable<ScanResultConsumer> {
+ private static final Logger logger =
LoggerFactory.getLogger(AsyncTableImpl.class);
Review comment:
We usually name it 'LOG'
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableImpl.java
##########
@@ -232,22 +236,35 @@ public ResultScanner getScanner(Scan scan) {
}
private void scan0(Scan scan, ScanResultConsumer consumer) {
- try (ResultScanner scanner = getScanner(scan)) {
- consumer.onScanMetricsCreated(scanner.getScanMetrics());
- for (Result result; (result = scanner.next()) != null;) {
- if (!consumer.onNext(result)) {
- break;
+ Span span = null;
+ try (AsyncTableResultScanner scanner = rawTable.getScanner(scan)) {
+ span = scanner.getSpan();
+ if (span == null) {
Review comment:
I do not think it could be null? Otherwise it is a bug in the async
client implementation?
--
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]
