Repository navigation
fix(pd): bound member lookups in RaftEngine.getMembers() - #3409
Open
ShryukGrandhi wants to merge 1 commit into
Open
ShryukGrandhi wants to merge 1 commit into
ShryukGrandhi wants to merge 1 commit into
Conversation
getMembers() waited on each peer's GetMemberRequest future without a timeout, so an unresponsive peer could stall /v1/members. Use the same rpc timeout as getLeaderGrpcAddress(), treat a null response as offline, and log the full exception when the bolt call fails. Signed-off-by: ShryukGrandhi <85529990+ShryukGrandhi@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose of the PR
On 1.7.0 every
GetMemberRequestbolt call failed with a message-less NPE becauseRaftRpcClientpassed a nullInvokeContext. Master already passesnew InvokeContext(), so that root cause is gone. Two related gaps from the issue are still on master:RaftEngine.getMembers()callsfuture.get()with no timeout for each peer (and checksisCompletedExceptionally()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.RaftRpcClientlogs onlyt.getMessage()when the bolt call throws. That is how the 1.7.0 failure showed up asfailed to call rpc to X. nullwith no stack trace.Main Changes
getMembers()now waits up toconfig.getRpcTimeout()per peer, the same boundgetLeaderGrpcAddress()got in fix(pd): add timeout and null-safety to getLeaderGrpcAddress() #2961. A timeout, an exception or a null response marks that peerOfflinewith its raft endpoint, and the remaining peers are still reported.RaftRpcClientpasses the exception to the logger, so the stack trace gets logged.Verifying these changes
RaftEngineLeaderAddressTest#testGetMembersMarksUnresponsivePeerOffline. One peer returns a future that never completes, another answers normally. Without the fix the test blocks infuture.get()and hits its 10s timeout. With the fix the first peer is reportedOfflineand the leaderUpwith 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?
Documentation Status
Doc - TODO: required documentation is pending; complete it before merging.Doc - Done: documentation is included here or linked below.Doc - No Need: no user-visible documentation is affected.