saintstack commented on a change in pull request #2813:
URL: https://github.com/apache/hbase/pull/2813#discussion_r549470855
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncRegionLocator.java
##########
@@ -84,64 +96,101 @@ private boolean isMeta(TableName tableName) {
return TableName.isMetaTableName(tableName);
}
+ private <T> CompletableFuture<T>
tracedLocationFuture(Supplier<CompletableFuture<T>> action,
+ Function<T, List<String>> getRegionNames, TableName tableName, String
methodName) {
+ Span span = createTableSpan(getClass().getSimpleName() + "." + methodName,
tableName);
+ try (Scope scope = span.makeCurrent()) {
+ CompletableFuture<T> future = action.get();
+ FutureUtils.addListener(future, (resp, error) -> {
+ if (error != null) {
+ span.recordException(error);
+ span.setStatus(StatusCode.ERROR);
+ } else {
+ List<String> regionNames = getRegionNames.apply(resp);
+ if (!regionNames.isEmpty()) {
+ span.setAttribute(REGION_NAMES_KEY, regionNames);
+ }
+ span.setStatus(StatusCode.OK);
+ }
+ span.end();
+ });
+ return future;
+ }
+ }
+
+ 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;
+ }
+
CompletableFuture<RegionLocations> getRegionLocations(TableName tableName,
byte[] row,
- RegionLocateType type, boolean reload, long timeoutNs) {
- CompletableFuture<RegionLocations> future = isMeta(tableName)
- ?
metaRegionLocator.getRegionLocations(RegionReplicaUtil.DEFAULT_REPLICA_ID,
reload)
- : nonMetaRegionLocator.getRegionLocations(tableName, row,
- RegionReplicaUtil.DEFAULT_REPLICA_ID, type, reload);
- return withTimeout(future, timeoutNs,
- () -> "Timeout(" + TimeUnit.NANOSECONDS.toMillis(timeoutNs) +
- "ms) waiting for region locations for " + tableName + ", row='" +
- Bytes.toStringBinary(row) + "'");
+ RegionLocateType type, boolean reload, long timeoutNs) {
+ return tracedLocationFuture(() -> {
+ CompletableFuture<RegionLocations> future = isMeta(tableName) ?
+
metaRegionLocator.getRegionLocations(RegionReplicaUtil.DEFAULT_REPLICA_ID,
reload) :
+ nonMetaRegionLocator.getRegionLocations(tableName, row,
+ RegionReplicaUtil.DEFAULT_REPLICA_ID, type, reload);
+ return withTimeout(future, timeoutNs,
+ () -> "Timeout(" + TimeUnit.NANOSECONDS.toMillis(timeoutNs) +
+ "ms) waiting for region locations for " + tableName + ", row='" +
+ Bytes.toStringBinary(row) + "'");
+ }, this::getRegionName, tableName, "getRegionLocations");
}
CompletableFuture<HRegionLocation> getRegionLocation(TableName tableName,
byte[] row,
- int replicaId, RegionLocateType type, boolean reload, long timeoutNs) {
- // meta region can not be split right now so we always call the same
method.
- // Change it later if the meta table can have more than one regions.
- CompletableFuture<HRegionLocation> future = new CompletableFuture<>();
- CompletableFuture<RegionLocations> locsFuture =
- isMeta(tableName) ? metaRegionLocator.getRegionLocations(replicaId,
reload)
- : nonMetaRegionLocator.getRegionLocations(tableName, row, replicaId,
type, reload);
- addListener(locsFuture, (locs, error) -> {
- if (error != null) {
- future.completeExceptionally(error);
- return;
- }
- HRegionLocation loc = locs.getRegionLocation(replicaId);
- if (loc == null) {
- future.completeExceptionally(
- new RegionOfflineException("No location for " + tableName + ",
row='" +
- Bytes.toStringBinary(row) + "', locateType=" + type + ",
replicaId=" + replicaId));
- } else if (loc.getServerName() == null) {
- future.completeExceptionally(
- new RegionOfflineException("No server address listed for region '" +
- loc.getRegion().getRegionNameAsString() + ", row='" +
Bytes.toStringBinary(row) +
- "', locateType=" + type + ", replicaId=" + replicaId));
- } else {
- future.complete(loc);
- }
- });
- return withTimeout(future, timeoutNs,
- () -> "Timeout(" + TimeUnit.NANOSECONDS.toMillis(timeoutNs) +
- "ms) waiting for region location for " + tableName + ", row='" +
Bytes.toStringBinary(row) +
- "', replicaId=" + replicaId);
+ int replicaId, RegionLocateType type, boolean reload, long timeoutNs) {
+ return tracedLocationFuture(() -> {
+ // meta region can not be split right now so we always call the same
method.
+ // Change it later if the meta table can have more than one regions.
+ CompletableFuture<HRegionLocation> future = new CompletableFuture<>();
+ CompletableFuture<RegionLocations> locsFuture =
+ isMeta(tableName) ? metaRegionLocator.getRegionLocations(replicaId,
reload) :
+ nonMetaRegionLocator.getRegionLocations(tableName, row, replicaId,
type, reload);
+ addListener(locsFuture, (locs, error) -> {
+ if (error != null) {
+ future.completeExceptionally(error);
+ return;
+ }
+ HRegionLocation loc = locs.getRegionLocation(replicaId);
+ if (loc == null) {
+ future.completeExceptionally(
+ new RegionOfflineException("No location for " + tableName + ",
row='" +
+ Bytes.toStringBinary(row) + "', locateType=" + type + ",
replicaId=" + replicaId));
+ } else if (loc.getServerName() == null) {
+ future.completeExceptionally(
+ new RegionOfflineException("No server address listed for region '"
+
+ loc.getRegion().getRegionNameAsString() + ", row='" +
Bytes.toStringBinary(row) +
+ "', locateType=" + type + ", replicaId=" + replicaId));
+ } else {
+ future.complete(loc);
+ }
+ });
+ return withTimeout(future, timeoutNs,
+ () -> "Timeout(" + TimeUnit.NANOSECONDS.toMillis(timeoutNs) +
+ "ms) waiting for region location for " + tableName + ", row='" +
+ Bytes.toStringBinary(row) + "', replicaId=" + replicaId);
+ }, loc -> Arrays.asList(loc.getRegion().getRegionNameAsString()),
tableName,
+ "getRegionLocation");
Review comment:
I like this clean insertion of trace future listener
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -203,26 +204,36 @@ public boolean isClosed() {
return closed.get();
}
+ private void closeQuietly(Closeable c) {
Review comment:
Why this refactor? I noticed that closeQuietly has been deprecated over
in commons-io for a long time (https://issues.apache.org/jira/browse/IO-504)
suggesting try/with-resources instead and that closeQuietly is never done right.
You thinking the AssertionError never happens? Or if it does, it is game
over?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncTableImpl.java
##########
@@ -218,50 +222,62 @@ private static Result toResult(HBaseRpcController
controller, MutateResponse res
@Override
public CompletableFuture<Result> get(Get get) {
- return timelineConsistentRead(conn.getLocator(), tableName, get,
get.getRow(),
- RegionLocateType.CURRENT, replicaId -> get(get, replicaId),
readRpcTimeoutNs,
- conn.connConf.getPrimaryCallTimeoutNs(), retryTimer,
conn.getConnectionMetrics());
+ return tracedFuture(
Review comment:
Good to keep the trace macro at first.... entrance and exit, yeah.
Simple. Later, when demand, can add trace across internal 'systems'... is the
time being taken adding to memstore or appending WAL -- that kinda thing.
So, yeah, entrance/exit on AsyncTable Interface methods would be good place
to start I'd think.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionImpl.java
##########
@@ -203,26 +204,36 @@ public boolean isClosed() {
return closed.get();
}
+ private void closeQuietly(Closeable c) {
Review comment:
Looks like this is a change in behavior. The default IOUtils seems to
have suppressed the IOE...
https://github.com/apache/commons-io/blob/18b80a32fc9979dbd23cc971093ac308e5fd77cc/src/main/java/org/apache/commons/io/IOUtils.java#L485
(I didn't go back to look at old impls).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
