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
HDDS-3101. Depend on lightweight ConfigurationSource interface instead of Hadoop Configuration #834
Conversation
Please review this PR even if it's conflicted. It's a small change, but big patch: I need to rebase it daily. |
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 @elek for this huge refactoring. Looks mostly good (216 / 216 files viewed, first time I used this GitHub review help feature), few questions/comments inline.
It's a small change, but big patch: I need to rebase it daily.
I would like to mention that a nontrivial part of the patch is cleanup of imports. While I'm not against that, it does increase the surface of merge conflicts unnecessarily.
...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
Show resolved
Hide resolved
...-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/SCMContainerManager.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/freon/OzoneGetConf.java
Outdated
Show resolved
Hide resolved
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/freon/OzoneGetConf.java
Outdated
Show resolved
Hide resolved
...gration-test/src/test/java/org/apache/hadoop/fs/ozone/contract/ITestOzoneContractCreate.java
Outdated
Show resolved
Hide resolved
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/fs/SpaceUsageCheckFactory.java
Outdated
Show resolved
Hide resolved
…m/container/SCMContainerManager.java Co-Authored-By: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>
Agree. I tried to follow an existing convention, but It turned out that there is no unified convention. Would be great to start a discussion to ozone-dev to use the same convention by anybody... |
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 @elek for updating the patch.
Thanks the review @adoroszlai I merged it to the master. |
What changes were proposed in this pull request?
To make it possible to create different client jars compiled with different version of Hadoop we need clear and Hadoop independent
hdds-common
(andhdds-client
) projects.(For more details about the motivation, check this design doc.)
Our current blocker is the usage of
org.apache.hadoop.conf.Configuration
. Configuration class is a heavyweight object fromhadoop-common
which introduce a lot of unnecessary dependencies. It also violates multiple OOP principles, for example the Dependency inversion principle (most of the time we don't need all the methods, it would be enough to depend on a reduced set of the methods)To make our components more independent I propose to depend on a lightweight
ConfigurationSource
interface which includes all the requiredgetXXX
methods from the existingConfiguration
. OzoneConfiguration can implement that interface (and with older Hadoop we can create direct adapters).Other motivation: Some of the used helper classes (related to the storage / time units) are not available in Hadoop 2.7. With this approach we can use any old Hadoop configuration as the only required method would be the
.get
method.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3101
How was this patch tested?
https://github.com/elek/hadoop-ozone/actions/runs/79732132