Copilot commented on code in PR #7105:
URL: https://github.com/apache/hbase/pull/7105#discussion_r2145345353
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBatchRpcRetryingCaller.java:
##########
@@ -218,8 +223,37 @@ private void logException(int tries,
Supplier<Stream<RegionRequest>> regionsSupp
String regions =
regionsSupplier.get().map(r -> "'" +
r.loc.getRegion().getRegionNameAsString() + "'")
.collect(Collectors.joining(",", "[", "]"));
- LOG.warn("Process batch for " + regions + " in " + tableName + " from "
+ serverName
- + " failed, tries=" + tries, error);
+ LOG.warn("Process batch for {} from {} failed, tries={}", regions,
serverName, tries, error);
+ }
+ }
+
+ @RestrictedApi(explanation = "Should only be called in tests", link = "",
+ allowedOnPath = ".*/(src/test/|AsyncBatchRpcRetryingCaller).*")
+ static void logException(int tries, int startLogErrorsCnt, RegionRequest
regionReq,
+ IdentityHashMap<Action, Throwable> action2Error, ServerName serverName) {
+ if (tries <= startLogErrorsCnt || action2Error.isEmpty()) {
+ return;
+ }
+ StringWriter sw = new StringWriter();
+ PrintWriter action2ErrorWriter = new PrintWriter(sw);
+ action2ErrorWriter.println();
+ Iterator<Map.Entry<Action, Throwable>> iter =
action2Error.entrySet().iterator();
+ for (int i = 0; i < 3 && iter.hasNext(); i++) {
Review Comment:
[nitpick] Consider extracting the hardcoded sampling limit `3` into a named
constant (e.g., `MAX_SAMPLED_ERRORS`) to improve readability and make it easier
to adjust later.
```suggestion
for (int i = 0; i < MAX_SAMPLED_ERRORS && iter.hasNext(); i++) {
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBatchRpcRetryingCaller.java:
##########
@@ -218,8 +223,37 @@ private void logException(int tries,
Supplier<Stream<RegionRequest>> regionsSupp
String regions =
regionsSupplier.get().map(r -> "'" +
r.loc.getRegion().getRegionNameAsString() + "'")
.collect(Collectors.joining(",", "[", "]"));
- LOG.warn("Process batch for " + regions + " in " + tableName + " from "
+ serverName
- + " failed, tries=" + tries, error);
+ LOG.warn("Process batch for {} from {} failed, tries={}", regions,
serverName, tries, error);
+ }
+ }
+
+ @RestrictedApi(explanation = "Should only be called in tests", link = "",
+ allowedOnPath = ".*/(src/test/|AsyncBatchRpcRetryingCaller).*")
+ static void logException(int tries, int startLogErrorsCnt, RegionRequest
regionReq,
+ IdentityHashMap<Action, Throwable> action2Error, ServerName serverName) {
+ if (tries <= startLogErrorsCnt || action2Error.isEmpty()) {
+ return;
+ }
+ StringWriter sw = new StringWriter();
+ PrintWriter action2ErrorWriter = new PrintWriter(sw);
+ action2ErrorWriter.println();
Review Comment:
[nitpick] This initial `println()` call introduces a leading blank line in
the logged message. If consistent log formatting is desired, consider removing
or relocating it.
```suggestion
```
##########
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncBatchRpcRetryingCaller.java:
##########
@@ -218,8 +223,37 @@ private void logException(int tries,
Supplier<Stream<RegionRequest>> regionsSupp
String regions =
regionsSupplier.get().map(r -> "'" +
r.loc.getRegion().getRegionNameAsString() + "'")
.collect(Collectors.joining(",", "[", "]"));
- LOG.warn("Process batch for " + regions + " in " + tableName + " from "
+ serverName
- + " failed, tries=" + tries, error);
+ LOG.warn("Process batch for {} from {} failed, tries={}", regions,
serverName, tries, error);
+ }
+ }
+
+ @RestrictedApi(explanation = "Should only be called in tests", link = "",
+ allowedOnPath = ".*/(src/test/|AsyncBatchRpcRetryingCaller).*")
+ static void logException(int tries, int startLogErrorsCnt, RegionRequest
regionReq,
+ IdentityHashMap<Action, Throwable> action2Error, ServerName serverName) {
+ if (tries <= startLogErrorsCnt || action2Error.isEmpty()) {
+ return;
+ }
+ StringWriter sw = new StringWriter();
+ PrintWriter action2ErrorWriter = new PrintWriter(sw);
+ action2ErrorWriter.println();
+ Iterator<Map.Entry<Action, Throwable>> iter =
action2Error.entrySet().iterator();
+ for (int i = 0; i < 3 && iter.hasNext(); i++) {
+ Map.Entry<Action, Throwable> entry = iter.next();
+ action2ErrorWriter.print(entry.getKey().getAction());
+ action2ErrorWriter.print(" => ");
+ entry.getValue().printStackTrace(action2ErrorWriter);
+ }
+ action2ErrorWriter.flush();
+ LOG.warn(
+ "Process batch for {} on {}, {}/{} actions failed, tries={}, sampled {}
errors are: {}",
+ regionReq.loc.getRegion().getRegionNameAsString(), serverName,
action2Error.size(),
+ regionReq.actions.size(), tries, Math.min(3, action2Error.size()),
sw.toString());
Review Comment:
[nitpick] The `StringWriter`/`PrintWriter` allocation and error sampling
logic only affect WARN logs. You could guard this block with `if
(LOG.isWarnEnabled())` to avoid allocations when WARN is disabled.
```suggestion
if (LOG.isWarnEnabled()) {
StringWriter sw = new StringWriter();
PrintWriter action2ErrorWriter = new PrintWriter(sw);
action2ErrorWriter.println();
Iterator<Map.Entry<Action, Throwable>> iter =
action2Error.entrySet().iterator();
for (int i = 0; i < 3 && iter.hasNext(); i++) {
Map.Entry<Action, Throwable> entry = iter.next();
action2ErrorWriter.print(entry.getKey().getAction());
action2ErrorWriter.print(" => ");
entry.getValue().printStackTrace(action2ErrorWriter);
}
action2ErrorWriter.flush();
LOG.warn(
"Process batch for {} on {}, {}/{} actions failed, tries={}, sampled
{} errors are: {}",
regionReq.loc.getRegion().getRegionNameAsString(), serverName,
action2Error.size(),
regionReq.actions.size(), tries, Math.min(3, action2Error.size()),
sw.toString());
}
```
--
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]
