ndimiduk commented on a change in pull request #4014:
URL: https://github.com/apache/hbase/pull/4014#discussion_r782667658
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java
##########
@@ -122,6 +135,37 @@ public boolean visitInternal(Result result) throws
IOException {
}
};
MetaTableAccessor.scanMetaForTableRegions(connection, visitor, tableName);
- return regions;
+ return consolidate(regions);
Review comment:
I think you are correct. I see now that `RegionLocations` has a
class-level javadoc, which explains some of this. I wish these methods
described their return values.
I have reverted these changes and `TestAdmin` passes locally. Let me push
that commit and see if it resolves all of the test failures.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AbstractRpcBasedConnectionRegistry.java
##########
@@ -272,6 +272,11 @@ private static RegionLocations
transformMetaRegionLocations(GetMetaRegionLocatio
getClass().getSimpleName() + ".getActiveMaster");
}
+ @Override
+ public String getConnectionString() {
+ return "unimplemented";
Review comment:
Arg, that's a great point... why did I do this? I did it on the master
PR as well. Let me get back to you on this.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java
##########
@@ -116,18 +122,30 @@ 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());
- }
+ static List<String> getRegionNames(RegionLocations locs) {
+ if (locs == null || locs.getRegionLocations() == null) {
+ return Collections.emptyList();
}
- return names;
+ return Arrays.stream(locs.getRegionLocations())
+ .filter(Objects::nonNull)
+ .map(HRegionLocation::getRegion)
+ .map(RegionInfo::getRegionNameAsString)
+ .collect(Collectors.toList());
+ }
+
+ static List<String> getRegionNames(HRegionLocation location) {
+ return Optional.ofNullable(location)
+ .map(HRegionLocation::getRegion)
+ .map(RegionInfo::getRegionNameAsString)
+ .map(Collections::singletonList)
Review comment:
Noted. I had not considered mutability of the previous implementation.
Consider this change to immutability a happy side-effect. At least on the
master PR, this appears to have introduced no issues. Perhaps this is cause for
some of the test failures I seem to have caused in the branch-2 backport. Let
me investigate.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/trace/TableSpanBuilder.java
##########
@@ -0,0 +1,99 @@
+/*
+ * 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 static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.DB_NAME;
+import static
org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.NAMESPACE_KEY;
+import static org.apache.hadoop.hbase.trace.HBaseSemanticAttributes.TABLE_KEY;
+import io.opentelemetry.api.common.AttributeKey;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanBuilder;
+import io.opentelemetry.api.trace.SpanKind;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Supplier;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.AsyncConnectionImpl;
+import org.apache.hadoop.hbase.client.ClusterConnection;
+import org.apache.hadoop.hbase.trace.TraceUtil;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Construct {@link Span} instances involving data tables.
+ */
[email protected]
+public class TableSpanBuilder implements Supplier<Span> {
Review comment:
An earlier version of this code made use of inheritance for these
builders and the template gymnastics required were awful. Following Duo's
suggestion, I undid that implementation in favor of this mix-in style approach
with reused utility methods. Lots of code that was DRY in the inheritance
version becomes repeated in this mix-in style. For me, I think this repeated
code is worth the hassle, because it'll be easier to maintain than the complex
hierarchy with esoteric template definitions.
All that said, if you see specific segments that could be extracted into
utility methods, I'm happy to make the improvements.
##########
File path:
hbase-client/src/test/java/org/apache/hadoop/hbase/client/trace/hamcrest/TraceTestUtil.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.hamcrest;
+
+import static
org.apache.hadoop.hbase.client.trace.hamcrest.AttributesMatchers.containsEntry;
+import static
org.apache.hadoop.hbase.client.trace.hamcrest.SpanDataMatchers.hasAttributes;
+import static org.hamcrest.Matchers.allOf;
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.sdk.trace.data.SpanData;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.AsyncConnectionImpl;
+import org.apache.hadoop.hbase.client.ConnectionImplementation;
+import org.hamcrest.Matcher;
+
+public final class TraceTestUtil {
+
+ private TraceTestUtil() { }
+
+ /**
+ * All {@link Span}s involving {@code conn} should include these attributes.
+ */
+ public static Matcher<SpanData>
buildConnectionAttributesMatcher(AsyncConnectionImpl conn) {
+ return hasAttributes(allOf(
+ containsEntry("db.system", "hbase"),
+ containsEntry("db.connection_string", "nothing"),
+ containsEntry("db.user", conn.getUser().toString())));
+ }
+
+ /**
+ * All {@link Span}s involving {@code conn} should include these attributes.
+ * @see #buildConnectionAttributesMatcher(AsyncConnectionImpl)
+ */
+ public static Matcher<SpanData>
buildConnectionAttributesMatcher(ConnectionImplementation conn) {
Review comment:
Yes, Both `ConnectionImplementation` and `AsyncConnectionImpl` became
public classes in order to facilitate their introspection for tracing. Not for
this test class, but for their use in the Builders. They remain `IA.Private`,
so I'm not concerned about leaking to user applications. I'm not familiar with
the conversation that lead to them being package private; do you have context
or specific concerns that can enlighten me?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/HRegionLocator.java
##########
@@ -122,6 +135,37 @@ public boolean visitInternal(Result result) throws
IOException {
}
};
MetaTableAccessor.scanMetaForTableRegions(connection, visitor, tableName);
- return regions;
+ return consolidate(regions);
+ }
+
+ private static RegionLocations consolidate(final List<RegionLocations>
locations) {
+ final HRegionLocation[] consolidated = locations.stream()
+ .filter(Objects::nonNull)
+ .flatMap(locs -> Arrays.stream(locs.getRegionLocations()))
+ .filter(Objects::nonNull)
+ .toArray(HRegionLocation[]::new);
Review comment:
Disregard. Per Huaxiang's comments, I'm reverting this method.
--
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]
