Conversation
ivandika3
marked this pull request as ready for review
September 18, 2026 14:37
Contributor
|
Please take a look at intermittent test failure, only seen in this PR so far. |
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.
What changes were proposed in this pull request?
While working on HDDS-15089, encountered an NPE issue that was caused by null
Server.getRemoteUser()inOzoneManager#getS3VolumeContext. This reason is that Ratis would callStateMachine#queryin a separate thread (underRaftServerImpl#clientExecutor). Therefore, theThreadLocalinOzoneManagerand theCurrCallwhich is used byServer.getRemoteUser()is not going to be propagated.This affects most read requests submitted to Ratis (executed in
OzoneManagerStateMachine#query) that require using ThreadLocal one way or another.We need a way to propagate the
Server#getCurrcallThreadLocalcontext to theStateMachine#query.Note this suggests that follower read feature is not production ready until this is resolved.
The main mechanism is the introduction of
OMRatisRequestContextwhich is a context mechanism to pass the OM ThreadLocal info OMRequest UserInfo and allows theOzoneManagerStateMachine#query(that is executed in a separate thread) to create am artificialServer.CallwithgetRemoteUserandgetHostInetAddressso thatServer.getRemoteUser()will not return null. Additionally, thisOmRatisRequestContextalso helps to handle propagation toOzoneManagerThreadLocallikestsTokenIdentifier.Note that OM might need to use the
RaftServer#readOnlyAsync(apache/ratis#1448) that might either be executed directly in caller thread (if we useDEFAULTReadOption) or executed in another thread (if we useLINEARIZABLEReadOption). Therefore,OMRatisRequestContextshould handle context propagation within a thread and across differentthreads (seepreviousCallandpreviousS3Context).I chose this approach since it does not need to change every OM read implementation. However, any suggestions to improve this design is welcome since this is a core OM logic.
This patch also includes some refactoring on
OMLockDetailsUtilandS3AuthenticationContextto reduce duplications.Saw projects like https://github.com/alibaba/transmittable-thread-local to pass the ThreadLocal context across threads, but the current solution should be enough for now. In the future, we should also think whether this ThreadLocal passing mechanisms should be a first-class feature for Ratis (or we can support something like ScopedValues in Ratis).
Generated by: GPT 5.6
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16497
How was this patch tested?
UT and IT.
Clean CI: https://github.com/ivandika3/ozone/actions/runs/35331947876