[GitHub] [hbase] Apache9 commented on a change in pull request #3952: HBASE-26474 Implement connection-level attributes

[GitBox <gi...@apache.org>]

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<String> getRegionName(RegionLocations locs) {
-    List<String> names = new ArrayList<>();
-    for (HRegionLocation loc : locs.getRegionLocations()) {
-      if (loc != null) {
-        names.add(loc.getRegion().getRegionNameAsString());
-      }
-    }
-    return names;
+  private static List<String> 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<Span> {
+public class TableOperationSpanBuilder<B extends TableOperationSpanBuilder<B>>
+  extends TableSpanBuilder<B> {
 
   // 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<AttributeKey<?>, 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-unsubscribe@hbase.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org