[GitHub] [hbase] Apache9 commented on a change in pull request #3952: HBASE-26474 Implement connection-level attributes
Apache9 commented on a change in pull request #3952:
URL: https://github.com/apache/hbase/pull/3952#discussion_r774244749
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/client/ShortCircuitConnectionRegistry.java
##
@@ -68,6 +68,11 @@ public
ShortCircuitConnectionRegistry(ConnectionRegistryEndpoint endpoint) {
return future;
}
+ @Override
+ public String getConnectionString() {
+return "localhost";
Review comment:
Maybe just call it short-circuit? I do not think localhost is accurate
enough, it may implies that we are connecting to a regsitry through network but
on the same machine? For short-circuit we know that we are using the
ShortCircuitConnectionRegistry.
--
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
[GitHub] [hbase] Apache9 commented on a change in pull request #3952: HBASE-26474 Implement connection-level attributes
Apache9 commented on a change in pull request #3952:
URL: https://github.com/apache/hbase/pull/3952#discussion_r772851451
##
File path:
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncRegionLocatorTracing.java
##
@@ -90,16 +102,33 @@ public void tearDown() throws IOException {
}
private SpanData waitSpan(String name) {
-Waiter.waitFor(CONF, 1000,
- () -> traceRule.getSpans().stream().map(SpanData::getName).anyMatch(s ->
s.equals(name)));
-return traceRule.getSpans().stream().filter(s ->
s.getName().equals(name)).findFirst().get();
+return waitSpan(hasName(name));
+ }
+
+ private SpanData waitSpan(Matcher matcher) {
+Matcher spanLocator = allOf(matcher, hasEnded());
+try {
+ Waiter.waitFor(CONF, 1000, new MatcherPredicate<>(
+"waiting for span",
+() -> traceRule.getSpans(), hasItem(spanLocator)));
+} catch (AssertionError e) {
Review comment:
Why catch AssertionError here?
And at lease, let's use LOG to output the message, not System.err?
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java
##
@@ -117,18 +123,33 @@ private boolean isMeta(TableName tableName) {
}
}
- private List getRegionName(RegionLocations locs) {
-List names = new ArrayList<>();
-for (HRegionLocation loc : locs.getRegionLocations()) {
- if (loc != null) {
-names.add(loc.getRegion().getRegionNameAsString());
- }
+ private static List getRegionNames(RegionLocations locs) {
+if (locs == null) {
Review comment:
if (locs == null || locs.getRegionLocations() == null)?
--
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
[GitHub] [hbase] Apache9 commented on a change in pull request #3952: HBASE-26474 Implement connection-level attributes
Apache9 commented on a change in pull request #3952:
URL: https://github.com/apache/hbase/pull/3952#discussion_r771051291
##
File path:
hbase-client/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableTracing.java
##
@@ -100,7 +99,9 @@
private ClientService.Interface stub;
- private AsyncConnection conn;
+ private User user;
Review comment:
Do we really need to declare it as a class member field? It is only used
in one method?
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java
##
@@ -117,18 +123,29 @@ private boolean isMeta(TableName tableName) {
}
}
- private List getRegionName(RegionLocations locs) {
-List names = new ArrayList<>();
-for (HRegionLocation loc : locs.getRegionLocations()) {
- if (loc != null) {
-names.add(loc.getRegion().getRegionNameAsString());
- }
-}
-return names;
+ private static List getRegionNames(RegionLocations locs) {
+if (locs == null) { return Collections.emptyList(); }
Review comment:
We still do not have an consensus on the style so let's keep the old
way? At least for me I do not link to have this style of '{}' block, as if the
line is too long, you still need to make a wrapping and then the code will be
like
```
if () {
xxx
}
```
but not
```
if () {
xxx }
```
Seems not consistent...
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/trace/TableOperationSpanBuilder.java
##
@@ -46,51 +44,61 @@
import org.apache.yetus.audience.InterfaceAudience;
/**
- * Construct {@link io.opentelemetry.api.trace.Span} instances originating from
- * "table operations" -- the verbs in our public API that interact with data
in tables.
+ * Construct {@link Span} instances originating from "table operations" -- the
verbs in our public
+ * API that interact with data in tables.
*/
@InterfaceAudience.Private
-public class TableOperationSpanBuilder implements Supplier {
+public class TableOperationSpanBuilder>
+ extends TableSpanBuilder {
// n.b. The results of this class are tested implicitly by way of the likes
of
// `TestAsyncTableTracing` and friends.
private static final String unknown = "UNKNOWN";
- private TableName tableName;
- private final Map, Object> attributes = new HashMap<>();
+ public TableOperationSpanBuilder(final AsyncConnectionImpl conn) {
+super(conn);
+ }
+
+ @Override
+ @SuppressWarnings("unchecked")
+ public B self() {
Review comment:
Why we need to have so complicated class hierarchy? A class has a
generic type parameter which is just itself?
--
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
