bitflicker64 commented on code in PR #3204:
URL: https://github.com/apache/hugegraph/pull/3204#discussion_r3987157390
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -411,6 +448,40 @@ <T> Optional<T> retryingInvoke(Supplier<T> supplier) {
}
+ /**
+ * Retry only failures that a fresh attempt can plausibly fix (a transport
error, a
+ * leader change). A DEADLINE_EXCEEDED would wait the full deadline again,
a CANCELLED
+ * means the caller's thread was interrupted; neither is retried, also
when it is one of
+ * several failures of a parallel commit.
+ */
+ static boolean isRetryable(Throwable t) {
+ return isRetryable(t, Collections.newSetFromMap(new
IdentityHashMap<>()));
+ }
+
+ private static boolean isRetryable(Throwable t, Set<Throwable> seen) {
+ Throwable c = t;
+ while (c != null && seen.add(c)) {
+ if (c instanceof InterruptedException) {
+ return false;
+ }
+ if (c instanceof StatusRuntimeException) {
+ Status.Code code = ((StatusRuntimeException)
c).getStatus().getCode();
+ if (code == Status.Code.DEADLINE_EXCEEDED || code ==
Status.Code.CANCELLED) {
Review Comment:
⚠️ Not retrying `DEADLINE_EXCEEDED` also drops a leader-failover recovery.
The Javadoc's reasoning (a retry waits the full deadline again) holds when the
retry goes to the same store, which is not the case once the partition leader
has moved. This corrects my earlier summary, which only looked at node eviction.
Evidence:
- Every RPC error sends a `NOT_WORK` notice
(`NotifyingExecutor.java:117-123`, `:244-255`).
`HgStoreNodePartitionerImpl.notice()` then calls
`pdClient.invalidPartitionCache()` (`HgStoreNodePartitionerImpl.java:190-195`),
which reloads shard-group leaders from PD right away
(`ClientCache.java:220-230`, `:178-186`).
- The retried `doCommit()` supplier re-runs `doAction()` for every entry
(`NodeTxExecutor.java:132-135`), so it routes against the reloaded leaders
(`NodeTxSessionProxy.java:731-734`, `:862-866`).
- A new raft leader is elected after the 3 s election timeout
(`HgStoreEngineOptions.java:91`) and pushes its shard group to PD
(`PartitionEngine.java:605-681`), well inside the 100 s default
`grpc.timeout.seconds` (`HgStoreClientConfig.java:30`).
So with replicated partitions, attempt 1 used to reach the new leader; now
the call fails after attempt 0. Interrupted REST workers still stop after one
deadline either way, but callers with no timeout that interrupts them (async
jobs are only interrupted on cancel) lose the recovery. Was the SIGSTOP cluster
at the shipped `default-shard-count: 1`
(`hugegraph-pd/hg-pd-dist/src/assembly/static/conf/application.yml:95`)? With 3
replicas the before run should have recovered on attempt 1.
Requested change: allow at most one `DEADLINE_EXCEEDED` retry per
`retryingInvoke()` call (keep `CANCELLED` and `InterruptedException`
non-retryable), update the Javadoc to match, and make
`testDeadlineExceededIsNotRetried` expect two attempts.
--
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]
