SebastianGruza commented on code in PR #3204:
URL: https://github.com/apache/hugegraph/pull/3204#discussion_r3984259105
##########
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:
Done in 8adf522. `commitSessions()` now collects every session failure in a
`ConcurrentLinkedQueue` and throws one `HgStoreClientException` (first failure
unwrapped one level as before, the others attached as suppressed);
`isRetryable()` walks the suppressed failures as well as the cause chain
(identity-set guarded against cycles), so one `DEADLINE_EXCEEDED` among several
failures makes the attempt non-retryable regardless of which partition reported
first. Tests: `testMixedCommitFailuresAreNotRetried` (two sessions,
`UNAVAILABLE` + `DEADLINE_EXCEEDED` thrown fresh on every call, exactly one
attempt, both sessions rolled back, one suppressed) and
`testAllRetryableCommitFailuresAreRetried`.
##########
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:
Done in 8adf522 — the throwable is passed as the trailing argument.
##########
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:
Done in 8adf522 — `isRetryable()` is checked first, the attempt budget
second.
##########
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:
Done in 8adf522 — `testInterruptBeforeCallSkipsTheAttempt` sets the flag
before the call and asserts `attempts == 0` with the flag still set.
##########
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:
Done in 8adf522 — line wrapped, unused import removed; no added line exceeds
100 characters now.
--
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]
