-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDFS-17324. RBF: Router should not return nameservices that not enable observer r… #6412
Conversation
…ead in RpcResponseHeaderProto
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@@ -136,9 +136,9 @@ public class RouterRpcClient { | |||
/** Field separator of CallerContext. */ | |||
private final String contextFieldSeparator; | |||
/** Observer read enabled. Default for all nameservices. */ | |||
private final boolean observerReadEnabledDefault; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making these things static is not very good practice.
@@ -86,7 +86,7 @@ public void setResponseHeaderState(RpcResponseHeaderProto.Builder headerBuilder) | |||
} | |||
RouterFederatedStateProto.Builder builder = RouterFederatedStateProto.newBuilder(); | |||
namespaceIdMap.forEach((k, v) -> { | |||
if (v.get() != Long.MIN_VALUE) { | |||
if ((v.get() != Long.MIN_VALUE) && RouterRpcClient.isNamespaceObserverReadEligible(k)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you making all the other stuff static because of this?
I don't think this is clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reivew. Yes, you are right. I will try to initizalize this in RouterStateIdContext.java only.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri @simbadzina Cloud you have time to review this, thanks! |
...bf/src/test/java/org/apache/hadoop/hdfs/server/federation/router/TestObserverWithRouter.java
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
@simbadzina @slfan1989 Hello, sir. Can you review this, Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @LiuGuH for the contribution.
Description of PR
HDFS-17324 RBF: Router should not return nameservices that not enable observer read in RpcResponseHeaderProto
Router Observer Read is controled by RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_DEFAULT_KEY and RBFConfigKeys.DFS_ROUTER_OBSERVER_READ_OVERRIDES.
If a nameservice is not enable for observer read in Router, RpcResponseHeaderProto in Router should not return it.