-
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-13522: IPC changes to support observer reads through routers. #4311
Conversation
💔 -1 overall
This message was automatically generated. |
4d719a2
to
2b51acb
Compare
💔 -1 overall
This message was automatically generated. |
2b51acb
to
4286ba8
Compare
💔 -1 overall
This message was automatically generated. |
This pull request is followed up by #4127 |
4286ba8
to
4a3b31a
Compare
💔 -1 overall
This message was automatically generated. |
4a3b31a
to
43853ef
Compare
💔 -1 overall
This message was automatically generated. |
43853ef
to
41312cb
Compare
🎊 +1 overall
This message was automatically generated. |
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.
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/NameNodeProxiesClient.java
Outdated
Show resolved
Hide resolved
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java
Outdated
Show resolved
Hide resolved
...dfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionManager.java
Outdated
Show resolved
Hide resolved
...dfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionManager.java
Outdated
Show resolved
Hide resolved
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.
@simbadzina This feature is very helpful. If you don't have time, I'd be happy to do something for you to push this feature forward.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/NameNodeProxiesClient.java
Outdated
Show resolved
Hide resolved
...dfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionManager.java
Outdated
Show resolved
Hide resolved
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterStateIdContext.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/NameNodeProxiesClient.java
Outdated
Show resolved
Hide resolved
...dfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/ConnectionManager.java
Outdated
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/main/resources/hdfs-default.xml
Outdated
Show resolved
Hide resolved
@simbadzina @omalley @melissayou I create a draft PR, and have some different points, like:
I'm hope that we can continue push this Feature forward. Please help me review it. @simbadzina If you want to continue work for this feature, feel free to take over it. Thanks~ |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
82a59d0
to
38ee8b1
Compare
💔 -1 overall
This message was automatically generated. |
38ee8b1
to
7cad62f
Compare
💔 -1 overall
This message was automatically generated. |
7cad62f
to
97bd692
Compare
ef4d26c
to
7ef4074
Compare
💔 -1 overall
This message was automatically generated. |
7ef4074
to
283ee92
Compare
💔 -1 overall
This message was automatically generated. |
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java
Outdated
Show resolved
Hide resolved
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java
Outdated
Show resolved
Hide resolved
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java
Outdated
Show resolved
Hide resolved
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/NamespaceStateId.java
Outdated
Show resolved
Hide resolved
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/FederatedNamespaceIds.java
Outdated
Show resolved
Hide resolved
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/FederatedNamespaceIds.java
Outdated
Show resolved
Hide resolved
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/FederatedNamespaceIds.java
Outdated
Show resolved
Hide resolved
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/FederatedNamespaceIds.java
Outdated
Show resolved
Hide resolved
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/FederatedNamespaceIds.java
Outdated
Show resolved
Hide resolved
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterStateIdContext.java
Outdated
Show resolved
Hide resolved
283ee92
to
856f63a
Compare
💔 -1 overall
This message was automatically generated. |
856f63a
to
bbaf062
Compare
…propagate it between routers and clients.
bbaf062
to
30497ad
Compare
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.
I have a couple of top level concerns:
- All clients will get all of the namespaces in their federated state ids.
- If the number of name spaces goes over the maximum, currently we'll msync to all of the namespaces, which will destroy performance.
- It looks to me like Server.getCurCall() is set when both the AlignmentContexts are updated (from the NN and to the client). That implies that we could use them to make smaller changes in this patch and only include the namespaces that the specific client cares about.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcConstants.java
Outdated
Show resolved
Hide resolved
...s-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/NameNodeProxiesClient.java
Show resolved
Hide resolved
...rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/FederatedNamespaceIds.java
Outdated
Show resolved
Hide resolved
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/PoolAlignmentContext.java
Show resolved
Hide resolved
...-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcClient.java
Outdated
Show resolved
Hide resolved
...-hdfs-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterRpcServer.java
Outdated
Show resolved
Hide resolved
...-rbf/src/main/java/org/apache/hadoop/hdfs/server/federation/router/RouterStateIdContext.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
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.
Just a few minor formatting issues, but otherwise looks good.
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/RpcConstants.java
Outdated
Show resolved
Hide resolved
...p-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/ClientGSIContext.java
Outdated
Show resolved
Hide resolved
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.
+1 LGTM
Thank you, Simba!
💔 -1 overall
This message was automatically generated. |
…propagate it between routers and clients. Fixes apache#4311 Signed-off-by: Owen O'Malley <oomalley@linkedin.com>
…propagate it between routers and clients. Fixes apache#4311 Signed-off-by: Owen O'Malley <oomalley@linkedin.com>
The design doc https://issues.apache.org/jira/secure/attachment/13047193/observer_reads_in_rbf_proposal_simbadzina_v2.pdf is very helpful . Can anyone put it again? Thanks very much |
I've replaced the file with one with refreshed links. https://issues.apache.org/jira/secure/attachment/13062835/observer_reads_in_rbf_proposal_simbadzina_v2.pdf I've uploaded the third party reads pdf to Jira as well: https://issues.apache.org/jira/secure/attachment/13062830/rbf_observer_reads%20-%20third_party_reads.pdf |
…propagate it between routers and clients. Fixes apache#4311 Signed-off-by: Owen O'Malley <oomalley@linkedin.com>
…propagate it between routers and clients. Fixes apache#4311 Signed-off-by: Owen O'Malley <oomalley@linkedin.com> ACLOVERRIDE
…propagate it between routers and clients. Fixes apache#4311 Signed-off-by: Owen O'Malley <oomalley@linkedin.com>
Description of PR
IPC changes so that clients send their txn number to routers and in turn routers will forward this to the namenodes. The code to direct routers to the observer namenodes will be in a follow up PR.
How was this patch tested?
Ran unit tests in module in Intellij
For code changes: