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

STORM-2217: Make DRPC pure java #1800

Merged
merged 4 commits into from
Dec 19, 2016
Merged

STORM-2217: Make DRPC pure java #1800

merged 4 commits into from
Dec 19, 2016

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Nov 27, 2016

This sets up some things for using Jersey for the UI, logviewer, and DRPC. It is not perfect and will probably need some clean up over time as we try to pull in UI and logviewer changes.

It divides up some of the functionality in DRPC so we could pull DRPC server and the webapp pieces into a separate module to separate out the classpaths. I have not shaded Jersey because of this, but am happy to try if people want me to.

I rewrote a lot of the core DRPC functionality so that when we do go to a newer version of jetty that supports async we can make DRPC much more scaleable.

I would love feedback on what I have done

@revans2
Copy link
Contributor Author

revans2 commented Nov 27, 2016

Will rebase soon.

@revans2
Copy link
Contributor Author

revans2 commented Nov 28, 2016

Rebased the code

@revans2 revans2 force-pushed the STORM-2217 branch 2 times, most recently from ce24e7d to 0782363 Compare November 28, 2016 13:34
@ptgoetz
Copy link
Member

ptgoetz commented Nov 30, 2016

+1. Looks good.

@revans2
Copy link
Contributor Author

revans2 commented Dec 1, 2016

Rebased again

@revans2
Copy link
Contributor Author

revans2 commented Dec 1, 2016

@ptgoetz if this looks good as is I will look into shading Jersey.

@revans2
Copy link
Contributor Author

revans2 commented Dec 2, 2016

Shading Jersey is becoming rather difficult (lots of dependencies including aop and dependency injection. Splitting the DPRC server off into it's own location seems much simpler and less error prone, so I will spend some time on that instead.

@revans2
Copy link
Contributor Author

revans2 commented Dec 7, 2016

So shading is not really an option for jersey.
I was able to split the DRPC server off into its own package with tests, but packaging it up with the assembly plugin is proving to be difficult. If I upgrade to 3.0.0 of the assembly plugin I can use moduleSets and make it work, but I have to change {{storm-dist/binary}} to a multi-module build and move the code that actually packages the final release to a sub package under it. Oh and we leak two empty/useless jar files into the release package that should be ignored. moduleSets do not package pom modules. They require an artifact that is a file.

I really dislike all of these options. I see a few cleaner options, but they will require a lot more work.

  1. Move to gradle
  2. Change how we do shading so that the assembly subModule code works the way they intended it.
    This would involve essentially having a separate package/build for what we want shaded (i.e. storm-shaded-deps.jar)

If someone has a cleaner solution I am happy to do it, but I think option 2 is the best so far, although I don't like it all that much.

@revans2
Copy link
Contributor Author

revans2 commented Dec 7, 2016

One more option, still ugly, but with a lot less impact. I can do something similar to Hadoop. They use several different invocations of the maven assembly plugin into directories (predates moduleSets) and then have a separate script that produces the final release.

@revans2
Copy link
Contributor Author

revans2 commented Dec 7, 2016

@ptgoetz I have updated the packaging to have a separate directory for the DRPC server dependencies. I have run manual tests and everything works. The big difference is that apache-storm-2.0.0-SNAPSHOT.tar.gz and apache-storm-2.0.0-SNAPSHOT.zip are now under ./final-package/target/ instead of ./target I will try and look to see if there are any docs that I need to update around this, but I wanted to give everyone a chance to look and give feedback on it.

@ptgoetz
Copy link
Member

ptgoetz commented Dec 7, 2016

@revans2 Okay. Let me review the packaging changes.

@ptgoetz
Copy link
Member

ptgoetz commented Dec 8, 2016

Okay, that last commit answered a lot of questions I had regarding packaging. ;)

+1 I was able to build a distribution, unpack it, and run the drpc service.

@HeartSaVioR
Copy link
Contributor

+1 from me. All manual tests are OK: build, dist packaging, unpacking, running drpc service.

@asfgit asfgit merged commit 7c24627 into apache:master Dec 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants