bitflicker64 commented on code in PR #3204:
URL: https://github.com/apache/hugegraph/pull/3204#discussion_r3982450791
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -376,32 +378,44 @@ <T> Optional<T> retryingInvoke(Supplier<T> supplier) {
return IntStream.rangeClosed(0, NODE_MAX_RETRYING_TIMES).boxed()
.map(
i -> {
+ if
(Thread.currentThread().isInterrupted()) {
+ // The caller (e.g. a REST worker
hitting
+ // restserver.request_timeout) gave
up: stop
+ // retrying instead of holding its
thread.
+ throw HgStoreClientException.of(
+ "Interrupted before retry " +
i);
+ }
T buffer = null;
try {
buffer = supplier.get();
} catch (Throwable t) {
- if (i + 1 <= NODE_MAX_RETRYING_TIMES) {
- try {
- int sleepTime;
- // The first three times try
once every second
- if (i < 3) {
- sleepTime = 1;
- } else {
- // Subsequent incremental
- sleepTime = i - 1;
- }
- log.info("Waiting {} seconds "
+
- "for the next try.",
- sleepTime);
- Thread.sleep(sleepTime *
1000L);
- } catch (InterruptedException e) {
- log.error("Failed to sleep",
e);
- }
- } else {
+ if (i + 1 > NODE_MAX_RETRYING_TIMES) {
log.error(maxTryMsg, t);
throw HgStoreClientException.of(
t.getMessage(), t);
}
+ if (!isRetryable(t)) {
+ // A deadline or a cancellation
will not
+ // get better by waiting the full
deadline
+ // again; fail fast and let the
caller decide.
+ log.warn("Not retrying after: {}",
Review Comment:
๐งน This log drops the throwable, on what this change makes the *common*
failure path.
Evidence:
- `log.warn("Not retrying after: {}", t.getMessage())` passes only the
message, so nothing reaches the logger's throwable slot.
- The branch four lines up keeps it: `log.error(maxTryMsg, t)` (`:393`).
Before this PR that was the only place a store failure was logged with its
cause chain; after it, a stalled node takes this branch on the first attempt
and never reaches `:393`, so the nested `StatusRuntimeException` and its stack
trace are no longer in the server log at all.
Requested change: `log.warn("Not retrying after: {}", t.getMessage(), t);` โ
slf4j appends a trailing `Throwable` argument as the cause.
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -376,32 +378,44 @@ <T> Optional<T> retryingInvoke(Supplier<T> supplier) {
return IntStream.rangeClosed(0, NODE_MAX_RETRYING_TIMES).boxed()
.map(
i -> {
+ if
(Thread.currentThread().isInterrupted()) {
+ // The caller (e.g. a REST worker
hitting
+ // restserver.request_timeout) gave
up: stop
+ // retrying instead of holding its
thread.
+ throw HgStoreClientException.of(
+ "Interrupted before retry " +
i);
+ }
T buffer = null;
try {
buffer = supplier.get();
} catch (Throwable t) {
- if (i + 1 <= NODE_MAX_RETRYING_TIMES) {
- try {
- int sleepTime;
- // The first three times try
once every second
- if (i < 3) {
- sleepTime = 1;
- } else {
- // Subsequent incremental
- sleepTime = i - 1;
- }
- log.info("Waiting {} seconds "
+
- "for the next try.",
- sleepTime);
- Thread.sleep(sleepTime *
1000L);
- } catch (InterruptedException e) {
- log.error("Failed to sleep",
e);
- }
- } else {
+ if (i + 1 > NODE_MAX_RETRYING_TIMES) {
log.error(maxTryMsg, t);
Review Comment:
๐งน Ordering the attempt-budget check before the retryability check makes the
last attempt log the wrong reason.
Evidence:
- At `i == NODE_MAX_RETRYING_TIMES` the loop takes `:392-396` first, so a
`DEADLINE_EXCEEDED` on the final attempt is reported as `"the number of retries
reached the upper limit : 10"` (`maxTryMsg`, `:58-60`) even though the new gate
at `:397` is what should have described it.
- This is reachable in the scenario the PR targets: attempts 0-9 fail
`UNAVAILABLE` while a store is being replaced (still retried by design), then
attempt 10 hits the deadline on the new node.
Requested change: move the `if (!isRetryable(t))` block above the `i + 1 >
NODE_MAX_RETRYING_TIMES` block so each exit logs the reason that actually
applied.
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -376,32 +378,44 @@ <T> Optional<T> retryingInvoke(Supplier<T> supplier) {
return IntStream.rangeClosed(0, NODE_MAX_RETRYING_TIMES).boxed()
.map(
i -> {
+ if
(Thread.currentThread().isInterrupted()) {
+ // The caller (e.g. a REST worker
hitting
+ // restserver.request_timeout) gave
up: stop
+ // retrying instead of holding its
thread.
+ throw HgStoreClientException.of(
+ "Interrupted before retry " +
i);
+ }
T buffer = null;
try {
buffer = supplier.get();
} catch (Throwable t) {
- if (i + 1 <= NODE_MAX_RETRYING_TIMES) {
- try {
- int sleepTime;
- // The first three times try
once every second
- if (i < 3) {
- sleepTime = 1;
- } else {
- // Subsequent incremental
- sleepTime = i - 1;
- }
- log.info("Waiting {} seconds "
+
- "for the next try.",
- sleepTime);
- Thread.sleep(sleepTime *
1000L);
- } catch (InterruptedException e) {
- log.error("Failed to sleep",
e);
- }
- } else {
+ if (i + 1 > NODE_MAX_RETRYING_TIMES) {
log.error(maxTryMsg, t);
throw HgStoreClientException.of(
t.getMessage(), t);
}
+ if (!isRetryable(t)) {
Review Comment:
โ ๏ธ On a commit that spans more than one partition, whether this gate fires
is decided by a race, so the fix is not guaranteed to engage on the very
workload the issue describes.
Evidence:
- `doCommit()` commits every session in parallel and keeps only the
**first** throwable: `sessions.parallelStream().forEach(...)` with
`throwable.compareAndSet(null, t)` (`NodeTxExecutor.java:136-145`). Which
`ForkJoinPool` task wins is nondeterministic.
- That single throwable is the only one rethrown (`:154-161`), so it is the
only input `isRetryable()` ever sees for the whole commit โ the other nodes'
failures are discarded, not even suppressed.
- So for a commit touching a stalled node (`DEADLINE_EXCEEDED`) and a node
being replaced (`UNAVAILABLE`, the #3130 recovery this PR is careful to
preserve): if the `UNAVAILABLE` lands first, the commit is retried up to 11
times and each attempt still blocks on the stalled partition for the full
`grpc.timeout.seconds` (`AbstractGrpcClient.java:128`). That is `11 ร
grpc.timeout.seconds` of held caller thread โ the stall from #3199, unchanged.
- Nothing pins this: all four new tests use a supplier with a single failure
mode, and `testTransientFailureIsStillRetried` /
`testDeadlineExceededIsNotRetried` each exercise one node's worth of behaviour.
Requested change: collect every failure instead of the first (a
`ConcurrentLinkedQueue<Throwable>` or `Collections.synchronizedList`) and make
the decision deterministic โ retry only when *every* captured failure is
retryable, attaching the rest as suppressed on the thrown
`HgStoreClientException` โ and add a test for a two-session commit where one
session fails `DEADLINE_EXCEEDED` and the other `UNAVAILABLE`, asserting
exactly one attempt.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutorTest.java:
##########
@@ -115,4 +123,67 @@ public void testParallelReplacementUsesOneCurrentSession()
throws Exception {
workers.shutdownNow();
}
}
+
+ @Test
+ public void testIsRetryableClassifiesFailures() {
+
assertFalse(NodeTxExecutor.isRetryable(Status.DEADLINE_EXCEEDED.asRuntimeException()));
+
assertFalse(NodeTxExecutor.isRetryable(Status.CANCELLED.asRuntimeException()));
+ assertFalse(NodeTxExecutor.isRetryable(new
InterruptedException("interrupted")));
+ // The status is usually wrapped by the time it reaches the retry loop
+ assertFalse(NodeTxExecutor.isRetryable(HgStoreClientException.of(
+ "commit failed", new
RuntimeException(Status.DEADLINE_EXCEEDED.asRuntimeException()))));
+
assertTrue(NodeTxExecutor.isRetryable(Status.UNAVAILABLE.asRuntimeException()));
+ assertTrue(NodeTxExecutor.isRetryable(new RuntimeException("simulated
transport failure")));
+ }
+
+ @Test
+ public void testDeadlineExceededIsNotRetried() {
+ NodeTxExecutor executor = NodeTxExecutor.graphOf("graph", null);
+ AtomicInteger attempts = new AtomicInteger();
+ HgStoreClientException e = assertThrows(HgStoreClientException.class,
() ->
+ executor.retryingInvoke(() -> {
+ attempts.incrementAndGet();
+ throw Status.DEADLINE_EXCEEDED.withDescription("deadline
exceeded after 20s")
+ .asRuntimeException();
+ }));
+ assertEquals(1, attempts.get());
+ assertTrue(e.getMessage(),
e.getMessage().contains("DEADLINE_EXCEEDED"));
+ }
+
+ @Test
+ public void testInterruptStopsRetrying() {
+ // A REST worker hitting restserver.request_timeout (or a Gremlin
evaluationTimeout)
+ // interrupts the calling thread while the store call is failing; the
loop must
+ // stop instead of sleeping and retrying with the interrupt swallowed.
+ NodeTxExecutor executor = NodeTxExecutor.graphOf("graph", null);
+ AtomicInteger attempts = new AtomicInteger();
+ try {
+ assertThrows(HgStoreClientException.class, () ->
+ executor.retryingInvoke(() -> {
+ attempts.incrementAndGet();
+ Thread.currentThread().interrupt();
+ throw new RuntimeException("simulated transport
failure");
+ }));
+ assertEquals(1, attempts.get());
Review Comment:
๐งน The new pre-attempt interrupt guard has no coverage; this test exercises
only the sleep handler.
Evidence:
- The supplier increments `attempts`, *then* sets the flag, then throws
(`:163-165`). So the `i == 0` guard at `NodeTxExecutor.java:381-387` runs on a
clean thread, `isRetryable(new RuntimeException(...))` is true, and the abort
comes from the `catch (InterruptedException)` around `Thread.sleep` at
`NodeTxExecutor.java:411-418`.
- This assertion proves it: `assertEquals(1, attempts.get())`. Had the guard
fired, the supplier would never have run and `attempts` would be `0`.
- The untested branch is the one that matters for a caller whose
`restserver.request_timeout` expired *between* store calls rather than during
one โ the case the guard's own comment cites.
Requested change: add a case that sets the flag before the call, e.g.
`Thread.currentThread().interrupt(); assertThrows(HgStoreClientException.class,
() -> executor.retryingInvoke(() -> { attempts.incrementAndGet(); return "ok";
})); assertEquals(0, attempts.get());`, with the same `finally {
Thread.interrupted(); }` guard this test already uses.
##########
hugegraph-store/hg-store-test/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutorTest.java:
##########
@@ -115,4 +123,67 @@ public void testParallelReplacementUsesOneCurrentSession()
throws Exception {
workers.shutdownNow();
}
}
+
+ @Test
+ public void testIsRetryableClassifiesFailures() {
+
assertFalse(NodeTxExecutor.isRetryable(Status.DEADLINE_EXCEEDED.asRuntimeException()));
+
assertFalse(NodeTxExecutor.isRetryable(Status.CANCELLED.asRuntimeException()));
+ assertFalse(NodeTxExecutor.isRetryable(new
InterruptedException("interrupted")));
+ // The status is usually wrapped by the time it reaches the retry loop
+ assertFalse(NodeTxExecutor.isRetryable(HgStoreClientException.of(
+ "commit failed", new
RuntimeException(Status.DEADLINE_EXCEEDED.asRuntimeException()))));
Review Comment:
๐งน Two style nits in the new test code.
Evidence:
- This line is 104 characters, over the project's `LineLength` max of 100
(`style/checkstyle.xml:29-32`). It is the only added line in the diff that
exceeds it.
- `import io.grpc.StatusRuntimeException` (`:43`) is unused: the tests only
call `Status.<CODE>.asRuntimeException()` and never name the type.
`UnusedImports` is enabled at `style/checkstyle.xml:59`.
- Neither breaks the build โ the checkstyle plugin is bound only in
`hugegraph-server/pom.xml:295` and `hugegraph-commons/pom.xml:147`, not in the
store modules โ so this is purely to keep the new code inside the shared style.
Requested change: wrap the argument list onto a second line and drop the
unused import.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
