ShryukGrandhi opened a new pull request, #3409:
URL: https://github.com/apache/hugegraph/pull/3409
## Purpose of the PR
- Fixes #3172
On 1.7.0 every `GetMemberRequest` bolt call failed with a message-less NPE
because `RaftRpcClient` passed a null `InvokeContext`. Master already passes
`new InvokeContext()`, so that root cause is gone. Two related gaps from the
issue are still on master:
- `RaftEngine.getMembers()` calls `future.get()` with no timeout for each
peer (and checks `isCompletedExceptionally()` before the get, which races with
completion). One unresponsive peer can stall `/v1/members`. A null response was
only handled by accident, through an NPE that got caught.
- `RaftRpcClient` logs only `t.getMessage()` when the bolt call throws. That
is how the 1.7.0 failure showed up as `failed to call rpc to X. null` with no
stack trace.
## Main Changes
- `getMembers()` now waits up to `config.getRpcTimeout()` per peer, the same
bound `getLeaderGrpcAddress()` got in #2961. A timeout, an exception or a null
response marks that peer `Offline` with its raft endpoint, and the remaining
peers are still reported.
- `RaftRpcClient` passes the exception to the logger, so the stack trace
gets logged.
## Verifying these changes
- [ ] Trivial rework / code cleanup without any test coverage. (No Need)
- [ ] Already covered by existing tests, such as *(please modify tests
here)*.
- [x] Need tests and can be verified as follows:
- Added
`RaftEngineLeaderAddressTest#testGetMembersMarksUnresponsivePeerOffline`. One
peer returns a future that never completes, another answers normally. Without
the fix the test blocks in `future.get()` and hits its 10s timeout. With the
fix the first peer is reported `Offline` and the leader `Up` with its gRPC URL.
- `mvn test -pl hugegraph-pd/hg-pd-test -am -Ppd-core-test`: 202 tests
run, 1 failure
(`RaftEngineReadIndexTest#testBackoffStartsOverAfterAnAnswerThatIsNotBusy`, a
timing assertion). It fails the same way on unmodified master on my machine and
is unrelated to this change.
## Does this PR potentially affect the following parts?
- [ ] Dependencies
- [ ] Modify configurations
- [ ] The public API
- [ ] Other affects (typed here)
- [x] Nope
## Documentation Status
- [ ] `Doc - TODO`: required documentation is pending; complete it before
merging.
- [ ] `Doc - Done`: documentation is included here or linked below.
- [x] `Doc - No Need`: no user-visible documentation is affected.
--
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]
