-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-16943. RBF: Implements MySQL based StateStoreDriver. #5469
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
Conversation
bcb399a
to
bd6a3b0
Compare
Using MySQL too is an external dependency, just like Zookeeper. I wonder if we should make StateStoreDrive IA.Public for others to extend it. Similar to MySQL, one should be able to use postgres and other databases too, but each impl might bring too much of external dependencies in hadoop, making the dependency management difficult over a period of time (more transitive CVE exposures?). WDYT? cc @goiri @slfan1989 |
I don't have much insights into Yarn federation but it looks like Yarn does have Perhaps keeping MySQL specific impl in test was meant to be set as an example only? @slfan1989 might have better insights. I don't mean anything against this change btw, I just wonder how we could manage ever increasing num of external dependencies such that we could allow more pluggable implementation to be used by different users without hadoop having to take up more external dependencies :) |
Yes, I agree it an external dependency. The desire it to depend on just one system MySQL for both the StateStore and DelegationTokens(HADOOP-18535). So StateStoreDriver is already public and can be configured with |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Correct, what I meant above by this "StateStoreDrive IA.Public for others to extend it" is that we can make |
I see. I think that is a good idea. With that change do you think the MySQLStateStore would still be valuable to the community? |
@virajjasani for making the StateStoreDrvier interface public, do you think we should then also do the same for |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
...bf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/StateStoreDriver.java
Show resolved
Hide resolved
...ain/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreMySQLImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreMySQLImpl.java
Outdated
Show resolved
Hide resolved
🎊 +1 overall
This message was automatically generated. |
@virajjasani I've annotated the StateStoreDriver interfaces as Public. Could you please do another review of the code. |
...bf/src/main/java/org/apache/hadoop/hdfs/server/federation/store/driver/StateStoreDriver.java
Show resolved
Hide resolved
...main/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreBaseImpl.java
Show resolved
Hide resolved
@simbadzina sorry I had the comment but somehow I forgot to complete the review, hence they didn't come to the PR. |
No worries, thanks for the review. |
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.
Left minor nits, looks good otherwise
...ain/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreMySQLImpl.java
Outdated
Show resolved
Hide resolved
...ain/java/org/apache/hadoop/hdfs/server/federation/store/driver/impl/StateStoreMySQLImpl.java
Show resolved
Hide resolved
+1 (non-binding), pending QA. Thanks @simbadzina |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@tasanuma could you please take a look at this pull request when you can. Thanks. |
Thanks @goiri for the review and approval. Could you please squash and merge the commits when appropriate. |
@virajjasani do you mind approving? |
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 (non-binding)
Sorry I did +1 but it was only comment. The changes look good for 3.4.0. Thank you @goiri @simbadzina !! |
Thank you for the reviews @virajjasani @goiri . |
(Cherry-picked from 47c22e3) ACLOVERRIDE
Description of PR
RBF supports two types of StateStoreDrivers
This PR implements a third driver that is backed by MySQL.
HADOOP-18535 implemented a MySQL token store. When tokens are stored in MySQL, using MySQL for the StateStore as well reduces the number of external dependencies for routers.
How was this patch tested?
New tests in TestStateStoreMySQL
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?