Skip to content
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-3353. Remove unnecessary transitive hadoop-common dependencies on server side. #781

Closed
wants to merge 1 commit into from

Conversation

elek
Copy link
Member

@elek elek commented Apr 7, 2020

What changes were proposed in this pull request?

Similar to HDDS-3312 we can exclude dependencies coming from hadoop-ozone. (Eg. curator / zookeeper.)

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-3353

Implementation details

The main idea is to use a hadoop-dependency-server project (similar to HDDS-3312) where we can exclude all the unnecessary dependencies and use this technical project instead of hadoop-common everywhere (server side).

One old class is removed which is not required any more. (It was easier than fixing a dependency problem).

Tests also required a hadoop-dependency-test umbrella project. (Without this one some transitive dependencies of the hadoop-common:test-jar were on a higher level and the scope was upgraded to test from compile).

How was this patch tested?

CI: https://github.com/elek/hadoop-ozone/actions/runs/72677849

  • find -name "*curator*.jar"

@elek elek changed the title fix server-side dependencies HDDS-3353. Remove unnecessary transitive hadoop-common dependencies on server side. Apr 7, 2020
@fapifta
Copy link
Contributor

fapifta commented Apr 7, 2020

Hi Marton, thank you for taking this on and looking into my suggestion deeper.

I ran a search for the string curator in the code, and found one more thing. I have added a pull request against your fork with the fix, please review and commit it if you agree.

The change to have a centralized dependency for the server and for the tests is a way I really like, and I am +1 on that part of the implementation.

I have done a quick check where we use hadoop-common as a dependency, and how we manage it there, and the followings came up:

  • mini-chaos-tests, insight, integration-tests, tools, common, container-service, client: are list hadoop-common as a dependency, we may use the new tests dependency here
  • hadoop-ozone-fs, csi: pom mentions hadoop-common but as an exclusion this is fine
    This is on its way as a PR to your branch also, after a quick local build.

On the other hand, I am unsure whether after these changes we need the hadoop-common dependency to be listed in the DependencyManagement section of the main pom.xml, as we use the hadoop.version property in the 3 remaining place in hadoop-dependency-server, in hadoop-dependency-test, and in hadoop-dependency-client and all modules now depends on these pseudo modules. My guess is we need it, but I am not that deeply familiar with dependency resolution to understand this easily so I thought to ask first. ;)

Copy link
Contributor

@adoroszlai adoroszlai left a 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 cleanup.

I would like to suggest merging this as it is, and ask @fapifta open a PR against apache/hadoop-ozone to get credit for the addendum.

@elek
Copy link
Member Author

elek commented Apr 8, 2020

I would like to suggest merging this as it is, and ask @fapifta open a PR against apache/hadoop-ozone to get credit for the addendum.

Good idea, especially because @fapifta's proposals are also very good.

Thanks to review to both of you. I am merging it and let's make an official PR/jira from the addendum for further improvements...

@elek elek closed this in 89c34b4 Apr 8, 2020
@fapifta
Copy link
Contributor

fapifta commented Apr 8, 2020

Right, thank you guys, I will create the JIRA and the PR later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants