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-3546. use shade plugin to build legacy jar #989

Closed
wants to merge 1 commit into from

Conversation

simonsssu
Copy link
Contributor

What changes were proposed in this pull request?

image

Currently, when using o3fs, it will throw exception like above pic shows, it's because the dependency hadoop-ozone-filesystem-lib-legacy-$version.jar using maven dependency plugin to build, it picks all the dependencies , unpack then into a jar. However, this will make some configuration file be overlap because of the same file name, eg. ozone-default-generated.xml. When we use o3fs, it will read default configuration value of SCMClientConfig in ozone-default-generated.xml if we don't config in ozone-site.xml, however, ozone-default-generated.xml is not completed in legacy jar.
So I use maven shaded plugin to replace maven dependency plugin, just like what hadoop-ozone-filesystem-lib-current does. The only difference is 'current' module will exclude some dependencies.

What is the link to the Apache JIRA

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

How was this patch tested?

  1. Build the legacy jar.
  2. Replace legacy jar in current cluster.
  3. remove configuration "scm.container.client.idle.threshold" and "scm.container.client.max.size"
  4. restart scm.
  5. try o3fs, and it will not throw "client is null" exception

@simonsssu simonsssu changed the title HDDS-3546. use shade to build legacy jar HDDS-3546. use shade plugin to build legacy jar May 29, 2020
@elek
Copy link
Member

elek commented May 29, 2020

Thanks to open this issue @simon0806

I agree, that it's a problem that the defaults are not merged currently and we should fix it.

  1. Small comments: As I see the original structure (storing the classes under libs/) is removed. It was requried for the isolated class loader. And it's not sure how can it work.

  2. Currently the hadoop-27 acceptane tests are broken, I am not sure if legacy works well. (If I understood well your steps under the test section, this is just the first problem).

  3. A bigger comment: we decided to remove this legacy magic all together. (There is a big limitation: Legacy with isloated class loader can't support secure Hadoop).

The work to remove legacy and replace with something simpler started weeks ago (See related issue and mailing list thred), and fortunatelly it closes to the end. The final patch is uploaded under #992.

If you are looking for a working, Hadoop 2.7 compatible client, give it a try. I would appreciate any help with the testing. This patch remove the Isolated/Filtered class loader from the picture and creates hadoop2.x compatible jar files.

If you really need this patch, I am fine to merge it somehow, but it requires more testing as the acceptance test of hadoop27-ozone is turned off. For me, it seems to be less effort to test and use #992, but I am open to any suggestion.

@simonsssu
Copy link
Contributor Author

simonsssu commented May 31, 2020

Hi elek, thanks for your reply, Actually I think this is not a high priority fix because we can still put the necessary configurations in ozone-site.xml, I can test your patch in my cases to see if it works well. on the other hand, I have another question, currently we use java annotation to generate the default configuration, so for some cases it will load the default value, I think it's a little complex because it depends on the maven compile. my suggestion is to make a unify configuration class and put default value on it, this way we don't need to generate configuration value during compile, just like what HiveConf.java in hive project does , what do you think ? @elek

@elek
Copy link
Member

elek commented Jun 2, 2020

, I think it's a little complex because it depends on the maven compile. my suggestion is to make a unify configuration class and put default value on it, this way we don't need to generate configuration value during compile, just like what HiveConf.java in hive project does , what do you think ? @elek

I am not sure if I understand the fully (I sorry if I misunderstood).

  1. Strictly speaking it depends on java compile not maven compile, annotation processors can work together with any javac
  2. I think one goal was to keep the defaults and the configuration definitions at same place (earlier we had Constants + XML. We kept it to different place.). I think the compilation step adds only a little complexity, but it has huge benefit to keep all the defaults together with the configuraiton.

One possible usage: With this approach we can print out easily all the default configuration (or all the configuration which have no default) for each of the projects (om/scm/common/client...)

If we would like to avoid the compilation phase, we can try to override the initialization of Configuration. As of now we load them from the fragments (OzoneConfiguration.loadDefaults), but it would be possible to directly read from the @ConfigGroup classes instead of parsing XML. (Yes, we need the list of the annotated classes in this case which either requires annotation processor or a list of the annotated classes).

@nandakumar131 nandakumar131 added the build Pull request that modifies the build process label Jun 4, 2020
@elek
Copy link
Member

elek commented Jun 17, 2020

/pending

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

/pending

@elek
Copy link
Member

elek commented Jun 23, 2020

Closing this issue. I checked and the original problem is not present on master:

unzip -p  target/hadoop-ozone-filesystem-hadoop3-0.6.0-SNAPSHOT.jar ozone-default-generated.xml

I can see config entries from ScmConfig and RatisClientConfig --> the merge works well.

There were some other proposals here, how the current process can be improved, I am open to continue the discussion but I suggest to open a separated issue (just to not block 0.6.0 with this issue)

@elek elek closed this Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Pull request that modifies the build process pending
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants