bitflicker64 commented on code in PR #3204:
URL: https://github.com/apache/hugegraph/pull/3204#discussion_r3995474168
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -373,35 +398,70 @@ boolean ifAnyTrue(Supplier<Stream<HgPair<HgStoreNode,
NodeTkv>>> nodeStreamSuppl
}
<T> Optional<T> retryingInvoke(Supplier<T> supplier) {
+ boolean[] deadlineRetried = {false};
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.
+ // InterruptedException as the root
cause: the
+ // server's task cancel path
recognises it
+ // (HugeException.isInterrupted()).
+ throw HgStoreClientException.of(
+ "Interrupted before retry " +
i,
+ new InterruptedException());
+ }
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);
+ Failure failure = classify(t);
+ if (failure == Failure.FATAL) {
+ // The caller's thread was
interrupted or the
+ // call was cancelled: fail fast.
+ log.warn("Not retrying after: {}",
+ t.getMessage(), t);
+ throw HgStoreClientException.of(
+ t.getMessage(), t);
+ }
+ if (failure == Failure.DEADLINE) {
+ // One retry: the NOT_WORK notice
sent for the
+ // failed RPC reloads the
partition leaders, so
+ // the next attempt can reach a
new leader. A
+ // second deadline in a row would
only wait the
+ // full deadline again on the same
stalled store.
+ if (deadlineRetried[0]) {
+ log.warn("Not retrying a
second deadline: {}",
+ t.getMessage(), t);
+ throw
HgStoreClientException.of(
+ t.getMessage(), t);
}
- } else {
+ deadlineRetried[0] = true;
+ log.warn("Deadline exceeded,
retrying once in " +
Review Comment:
⚠️ The PR description still describes the pre-`b588aa88` behaviour, so the
numbers reviewers and the squash commit message will carry are wrong for this
head.
Evidence:
- The body says "Do not retry a failure whose cause chain carries
`Status.Code.DEADLINE_EXCEEDED` … (`isRetryable()`)". At this head a deadline
**is** retried once (this block, plus the guard four lines down), and
`isRetryable()` no longer exists: `git grep -n isRetryable 172b2d0 --
hugegraph-store` returns nothing — it is `classify()` / `Failure` now.
- The before/after table's `slowest write | 20.1 s` was measured with
`grpc.timeout.seconds=20` under the never-retry behaviour, i.e. one deadline.
With the single retry the worst case is two deadlines plus the 1 s backoff:
about 41 s in that setup, and about 201 s on the shipped default
`grpc.timeout.seconds=100`. That is still a large improvement over `11 ×
timeout + 38 s`, but it is twice what the table claims.
Requested change: update the "Main Changes" bullet to describe the
retry-once classification and rename `isRetryable()` to `classify()`, and
either re-run the SIGSTOP measurement at `172b2d0` or annotate the table as
taken at `920bbbdf`.
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -373,35 +398,70 @@ boolean ifAnyTrue(Supplier<Stream<HgPair<HgStoreNode,
NodeTkv>>> nodeStreamSuppl
}
<T> Optional<T> retryingInvoke(Supplier<T> supplier) {
+ boolean[] deadlineRetried = {false};
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.
+ // InterruptedException as the root
cause: the
+ // server's task cancel path
recognises it
+ // (HugeException.isInterrupted()).
+ throw HgStoreClientException.of(
+ "Interrupted before retry " +
i,
+ new InterruptedException());
+ }
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);
+ Failure failure = classify(t);
+ if (failure == Failure.FATAL) {
+ // The caller's thread was
interrupted or the
+ // call was cancelled: fail fast.
+ log.warn("Not retrying after: {}",
+ t.getMessage(), t);
+ throw HgStoreClientException.of(
+ t.getMessage(), t);
+ }
+ if (failure == Failure.DEADLINE) {
+ // One retry: the NOT_WORK notice
sent for the
+ // failed RPC reloads the
partition leaders, so
+ // the next attempt can reach a
new leader. A
+ // second deadline in a row would
only wait the
+ // full deadline again on the same
stalled store.
+ if (deadlineRetried[0]) {
Review Comment:
🧹 The comment and the javadoc say "a second deadline **in a row**", but the
flag is never reset, so the code means "a second deadline anywhere in this
invocation".
Evidence:
- `boolean[] deadlineRetried = {false}` (`:401`) is set once at `:441` and
never cleared, including after an attempt that fails with something else or
that succeeds partially.
- Concretely: attempt 0 `DEADLINE_EXCEEDED` (retried), attempts 1-9
`UNAVAILABLE` (retried, ~38 s of backoff, partition leaders reloaded several
times over), attempt 10 `DEADLINE_EXCEEDED` — the guard fires and the call
aborts, even though these two deadlines are not consecutive and the stated
rationale ("the `NOT_WORK` notice … reloads the partition leaders") applies
just as much the second time.
- The same wording is in the `Failure` javadoc at `:478`.
Requested change: pick one. Either reset `deadlineRetried[0] = false`
whenever an attempt fails with a non-`DEADLINE` classification, which makes the
code match "in a row"; or reword `:433` and `:478` to "a second deadline in
this invocation" so the budget is honestly described as one deadline retry per
call.
##########
hugegraph-store/hg-store-client/src/main/java/org/apache/hugegraph/store/client/NodeTxExecutor.java:
##########
@@ -129,35 +136,7 @@ void doCommit() {
if (!allSuccess.get()) {
throw HgStoreClientException.of(msg);
}
- AtomicReference<Throwable> throwable = new AtomicReference<>();
- Collection<HgStoreSession> sessions = this.sessions.values();
- sessions.parallelStream().forEach(e -> {
- if (e.isTx()) {
- try {
- e.commit();
- } catch (Throwable t) {
- throwable.compareAndSet(null, t);
- allSuccess.set(false);
- }
- }
- });
- if (!allSuccess.get()) {
- if (isTx) {
- try {
-
sessions.stream().forEach(HgStoreSession::rollback);
- } catch (Exception e) {
-
- }
- }
- Throwable cause = throwable.get();
- if (cause.getCause() != null) {
- cause = cause.getCause();
- }
- if (cause instanceof HgStoreClientException) {
- throw (HgStoreClientException) cause;
- }
- throw HgStoreClientException.of(cause);
- }
+ this.commitSessions(this.sessions.values());
Review Comment:
🧹 Extracting `commitSessions()` removed the only writer of `allSuccess`,
leaving a dead variable and an unreachable branch.
Evidence:
- `AtomicBoolean allSuccess = new AtomicBoolean(true)` (`:131`) is now only
ever read. The `allSuccess.set(false)` that used to live in the inline
parallel-commit block moved into `commitSessions()`, which reports through the
thrown exception instead.
- So `if (!allSuccess.get()) { throw HgStoreClientException.of(msg); }`
(`:136-138`) can never fire, and `msg` is referenced only from that unreachable
throw.
Requested change: delete `:131` and `:136-138` (and the now-unused `msg`
field, plus the `java.util.concurrent.atomic.AtomicBoolean` import, if nothing
else uses them). If the gate was meant to catch a `doAction()` that returned
`false`, wire the loop's return value into it instead of leaving it dead.
--
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]
