ndimiduk commented on a change in pull request #4106:
URL: https://github.com/apache/hbase/pull/4106#discussion_r809115423
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableImpl.java
##########
@@ -44,12 +45,11 @@
@InterfaceAudience.Private
class AsyncTableImpl implements AsyncTable<ScanResultConsumer> {
- private final AsyncTable<AdvancedScanResultConsumer> rawTable;
+ private final RawAsyncTableImpl rawTable;
Review comment:
I think that changing this member variable to the concrete type makes
following the relationship between these two classes more clear -- should I
break this cleanup out into a separate PR?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncClientScanner.java
##########
@@ -140,26 +152,35 @@ public OpenScannerResponse(HRegionLocation loc, boolean
isRegionServerRemote, In
private CompletableFuture<OpenScannerResponse>
callOpenScanner(HBaseRpcController controller,
HRegionLocation loc, ClientService.Interface stub) {
- boolean isRegionServerRemote = isRemote(loc.getHostname());
- incRPCCallsMetrics(scanMetrics, isRegionServerRemote);
- if (openScannerTries.getAndIncrement() > 1) {
- incRPCRetriesMetrics(scanMetrics, isRegionServerRemote);
- }
- CompletableFuture<OpenScannerResponse> future = new CompletableFuture<>();
- try {
- ScanRequest request =
RequestConverter.buildScanRequest(loc.getRegion().getRegionName(), scan,
- scan.getCaching(), false);
- stub.scan(controller, request, resp -> {
- if (controller.failed()) {
- future.completeExceptionally(controller.getFailed());
- return;
- }
- future.complete(new OpenScannerResponse(loc, isRegionServerRemote,
stub, controller, resp));
- });
- } catch (IOException e) {
- future.completeExceptionally(e);
+ final Span localSpan = span;
+ try (Scope ignored = localSpan.makeCurrent()) {
+ boolean isRegionServerRemote = isRemote(loc.getHostname());
+ incRPCCallsMetrics(scanMetrics, isRegionServerRemote);
+ if (openScannerTries.getAndIncrement() > 1) {
+ incRPCRetriesMetrics(scanMetrics, isRegionServerRemote);
+ }
+ CompletableFuture<OpenScannerResponse> future = new
CompletableFuture<>();
+ try {
+ ScanRequest request = RequestConverter.buildScanRequest(
+ loc.getRegion().getRegionName(), scan, scan.getCaching(), false);
+ stub.scan(controller, request, resp -> {
+ try (Scope ignored1 = localSpan.makeCurrent()) {
+ if (controller.failed()) {
+ final IOException e = controller.getFailed();
+ future.completeExceptionally(e);
+ TraceUtil.setError(localSpan, e);
+ localSpan.end();
+ return;
+ }
+ future.complete(new OpenScannerResponse(
+ loc, isRegionServerRemote, stub, controller, resp));
+ }
+ });
+ } catch (IOException e) {
+ future.completeExceptionally(e);
Review comment:
Do we need to `setError` and `span.end()` here as well?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -362,7 +362,7 @@ ClientBackoffPolicy getBackoffPolicy() {
public AsyncTable<ScanResultConsumer> build() {
RawAsyncTableImpl rawTable =
new RawAsyncTableImpl(AsyncConnectionImpl.this, RETRY_TIMER, this);
- return new AsyncTableImpl(AsyncConnectionImpl.this, rawTable, pool);
+ return new AsyncTableImpl(rawTable, pool);
Review comment:
the `conn` instance is never used in `AsyncTableImpl` constructor --
should I break this cleanup out into a separate PR?
##########
File path:
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableTracing.java
##########
@@ -452,6 +459,53 @@ public void testScanAll() {
assertTrace("SCAN");
}
+ @Test
+ public void testScan() throws Throwable {
Review comment:
These two test methods are awfully inadequate. Let me replace this with
test classes that more closely resemble the children of
`AbstractTestAsyncTableScan`.
##########
File path:
hbase-client/src/test/java/org/apache/hadoop/hbase/client/trace/StringTraceRenderer.java
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.client.trace;
+
+import io.opentelemetry.api.trace.SpanId;
+import io.opentelemetry.sdk.trace.data.SpanData;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.ListIterator;
+import java.util.Map;
+import java.util.Objects;
+import java.util.function.Consumer;
+import java.util.stream.Collectors;
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.commons.lang3.builder.ToStringStyle;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A Rudimentary tool for visualizing a hierarchy of spans. Given a collection
of spans, indexes
+ * them from parents to children and prints them out one per line, indented.
+ */
Review comment:
This is used to produce a rudimentary dump of the spans collected by the
test rule. Its output looks like,
```
2022-02-16T17:10:02,100 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=f62afe5e0bd62fac,name=testTracing,hasEnded=true]
2022-02-16T17:10:02,100 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): └─ [spanId=52fd594bc0efd0cb,name=SCAN
default:async,hasEnded=true]
2022-02-16T17:10:02,101 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): └─
[spanId=07ef27e2c995d650,name=TracedAdvancedScanResultConsumer#onNext,hasEnded=true]
2022-02-16T17:10:02,101 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=7287673c2a5efddc,name=AsyncRegionLocator.getRegionLocation,hasEnded=true]
2022-02-16T17:10:02,101 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=a31f63eee579eaaa,name=AsyncRegionLocator.getRegionLocation,hasEnded=true]
2022-02-16T17:10:02,101 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=4aaffe13cafa2c14,name=TracedAdvancedScanResultConsumer#onComplete,hasEnded=true]
2022-02-16T17:10:02,101 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=3bb033095b8c7fad,name=AsyncRegionLocator.getRegionLocation,hasEnded=true]
2022-02-16T17:10:02,101 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=00c21a6a147dec07,name=TracedAdvancedScanResultConsumer#onNext,hasEnded=true]
2022-02-16T17:10:02,102 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=543db9d824329f18,name=hbase.pb.ClientService/Scan,hasEnded=true]
2022-02-16T17:10:02,102 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): └─
[spanId=31f7e3cd84777126,name=RpcServer.process,hasEnded=true]
2022-02-16T17:10:02,102 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): └─
[spanId=df64a090ab44d5c5,name=hbase.pb.ClientService/Scan,hasEnded=true]
2022-02-16T17:10:02,102 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): └─
[spanId=32702f65adf3f544,name=Region.getScanner,hasEnded=true]
2022-02-16T17:10:02,102 DEBUG [Time-limited test]
trace.StringTraceRenderer(98): ├─
[spanId=069c1a8c42e17543,name=AsyncRegionLocator.getRegionLocation,hasEnded=true]
```
##########
File path: pom.xml
##########
@@ -1884,12 +1884,13 @@
-Djava.security.egd=file:/dev/./urandom -Djava.net.preferIPv4Stack=true
-Djava.awt.headless=true
-Djdk.net.URLClassPath.disableClassPathURLCheck=true
-Dorg.apache.hbase.thirdparty.io.netty.leakDetection.level=advanced
- -Dio.netty.eventLoopThreads=3
+ -Dio.netty.eventLoopThreads=3
-Dio.opentelemetry.context.enableStrictContext=true
Review comment:
This is supposed to enable strict checking that scopes are closed
properly. I'm not convinced it works in practice.
https://github.com/open-telemetry/opentelemetry-java/blob/77f0b0adc4ab9748cb7acf72952ecea2b4fda5f6/context/src/main/java/io/opentelemetry/context/StrictContextStorage.java#L147
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableImpl.java
##########
@@ -233,22 +233,29 @@ 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();
+ try (Scope ignored = span.makeCurrent()) {
+ consumer.onScanMetricsCreated(scanner.getScanMetrics());
+ for (Result result; (result = scanner.next()) != null; ) {
+ if (!consumer.onNext(result)) {
+ break;
+ }
}
+ consumer.onComplete();
}
- consumer.onComplete();
} catch (IOException e) {
- consumer.onError(e);
+ try (Scope ignored = span.makeCurrent()) {
Review comment:
IntelliJ static analysis says that `span` can never be `null` here, but
reading the flow, I don't see how that can be true. What do you think?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableResultScanner.java
##########
@@ -57,12 +58,13 @@
private ScanResumer resumer;
- public AsyncTableResultScanner(AsyncTable<AdvancedScanResultConsumer> table,
Scan scan,
- long maxCacheSize) {
- this.rawTable = table;
+ // Used to pass the span instance to the `AsyncTableImpl` from its
underlying `rawAsyncTable`.
+ private Span span = null;
+
+ public AsyncTableResultScanner(TableName tableName, Scan scan, long
maxCacheSize) {
Review comment:
I think that pulling the `table.scan()` call out of the constructor is
better because it moves starting the scan machinery outside of the constructor
and back into control of the caller. In doing so, I can also remove the need
for the caller to pass in `this`, which reads to me as a code smell. Should I
break this cleanup out into a separate PR?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncTableImpl.java
##########
@@ -628,38 +628,38 @@ private long resultSize2CacheSize(long maxResultSize) {
}
@Override
- public ResultScanner getScanner(Scan scan) {
- return new AsyncTableResultScanner(this,
ReflectionUtils.newInstance(scan.getClass(), scan),
- resultSize2CacheSize(
- scan.getMaxResultSize() > 0 ? scan.getMaxResultSize() :
defaultScannerMaxResultSize));
+ public AsyncTableResultScanner getScanner(Scan scan) {
+ final long maxCacheSize = resultSize2CacheSize(
+ scan.getMaxResultSize() > 0 ? scan.getMaxResultSize() :
defaultScannerMaxResultSize);
+ final Scan scanCopy = ReflectionUtils.newInstance(scan.getClass(), scan);
+ final AsyncTableResultScanner scanner =
+ new AsyncTableResultScanner(tableName, scanCopy, maxCacheSize);
+ scan(scan, scanner);
+ return scanner;
}
@Override
public CompletableFuture<List<Result>> scanAll(Scan scan) {
- final Supplier<Span> supplier = newTableOperationSpanBuilder()
Review comment:
This change pushes span creation down into the machinery that is common
between `scan` and `scanAll`. The rest of the change here is just indentation.
##########
File path:
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableTracing.java
##########
@@ -106,7 +113,7 @@
private AsyncConnectionImpl conn;
- private AsyncTable<?> table;
+ private AsyncTable<ScanResultConsumer> table;
Review comment:
Specifying the generic type here makes it clear to a knowledgable reader
that this test covers `AsyncTableImpl` only, and only covers
`RawAsyncTableImpl` through their delegated relationship.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncTableResultScanner.java
##########
@@ -138,7 +149,7 @@ public synchronized Result next() throws IOException {
return null;
}
if (error != null) {
- FutureUtils.rethrow(error);
+ throw FutureUtils.rethrow(error);
Review comment:
There's a `throw` inside of the `rethrow` method ; adding one here make
this clear and silences a static analysis warning.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
