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-2441 Break down 'storm-core' to extract client (worker) artifacts #2034

Merged
merged 1 commit into from
Apr 6, 2017

Conversation

HeartSaVioR
Copy link
Contributor

@HeartSaVioR HeartSaVioR commented Mar 31, 2017

Sorry for the huge diff. I've generated classes from storm.thrift from new module, and git doesn't recognize moving files, or IDEA doesn't support git mv when refactor-move.
I'll try to rework via git mv if we really want to minimize the changeset.

Please read issue link and following discussion to see rationale of this issue before reviewing.

This PR breaks down storm-core into multiple artifacts.

  • storm-client: topology and worker, play as client SDK
  • storm-client-misc: client SDK misc which is not added due to dependency addition
    • depends on storm-client
  • storm-server: LocalCluster and related daemons (most of them, without webapp)
    • depends on storm-client
  • storm-core: Clojure things and unported daemons (webapp) and tests, and commands for storm.py
    • depends on storm-server (hence storm-client, too)
    • we could get rid of this after porting work is done, or at least rename to other name so that end users can notice that they should use storm-client instead of storm-core
  • storm-webapp: DRPC HTTP server, Logviewer and UI will be placed here
    • depends on storm-core (hence storm-client and storm-server, too)

After applying this patch, binary dist. will have three library directories, lib for storm-core (daemons), lib-worker for storm-client, lib-webapp for storm-webapp.

We could change lib to refer storm-server after porting is done, or even storm-webapp and remove lib-webapp.

Please note that there is a precondition to finish this work.

  • Decide how to get over dependency issue (need discussion)

This patch doesn't relocate what we have been relocating. Relocation doesn't work when we have multiple modules, and we already have multiple modules for storm-core and storm-drpc-server which shows build failure from IntelliJ IDEA.

Unit tests and manual tests passed.

commit log

  • break down 'storm-core' into multiple artifacts
    • 'storm-client': topology and worker, play as client SDK
    • 'storm-client-misc': client SDK misc which is not added due to dependency addition
    • 'storm-server': LocalCluster and related daemons
    • 'storm-core': Clojure things and unported daemons (webapp) and tests
    • 'storm-webapp': DRPC HTTP server, Logviewer and UI will be placed here
  • Supervisor will set classpath for worker to just ensure 'storm-client', not 'storm-core'
  • address documentation
  • apply the change to all modules's pom.xml
  • add new modules into Travis build
  • NOTE: we don't shade and relocate what we have been relocated
    • we should discuss how to overcome dependency issue

@HeartSaVioR HeartSaVioR force-pushed the STORM-2441 branch 2 times, most recently from 21ca748 to a7ec4ea Compare March 31, 2017 05:58
@HeartSaVioR
Copy link
Contributor Author

Travis build fails intermittently. Travis build for fork passes (without integration test).
https://travis-ci.org/HeartSaVioR/storm/builds/217048964

@HeartSaVioR
Copy link
Contributor Author

I googled about git mv and some SO article claims git mv is just a shorthand of delete and create. Unfortunately there seems no way to reduce the diff.

@revans2
Copy link
Contributor

revans2 commented Mar 31, 2017

I am +1 for the general concept. I need to finish going through the code, but I am probably going to do it outside of github because the patch is so huge.

As a side note git used to use a config to detect if a file moved. It would diff the files and if a small amount changed between the two it would consider it a move, but it was inexact and changed between clients (because the configured leniency could be different). Now I believe that it is metadata stored with the commit.

@revans2
Copy link
Contributor

revans2 commented Mar 31, 2017

I filed STORM-2447 and STORM-2448 to allow for us to separate out the classpath and make the change transparent for end users.

@revans2
Copy link
Contributor

revans2 commented Mar 31, 2017

+1 it looks good to me.

@revans2
Copy link
Contributor

revans2 commented Apr 1, 2017

Just to let you know that I have been working on making local mode transparent. I have it working, but I have a lot of work to do on the examples and docs left.

HeartSaVioR/storm@STORM-2441...revans2:STORM-2447

To make a long story short if you run storm local instead of storm jar it will bring up local mode cluster and local mode DRPC and hide it all fairly well from the end user.

Once I finish that I will start looking at working on letting a user select an alternative worker classpath.

@ptgoetz and @HeartSaVioR I would like to merge this into master sooner rather then later so we don't have to keep up-merging it. Please let me know if you have any concerns about this.

@HeartSaVioR
Copy link
Contributor Author

@revans2
We didn't address origin concern, dependencies issue. This patch doesn't relocate everything so it would be a regression for many users.
I'd like to get it resolved before 2.0.0 is released. If we can address it in this PR that would be best, but we could file a new issue for that with marking as 'blocker', and go on.

You know, I'm not familiar with Clojure so I couldn't sort out Clojure things in this PR. storm-clojure still rely on storm-core rather than storm-client (or even storm-server). If someone helps me to sort out Clojure side that would be also great. Maybe we could address this from new issue too.

@HeartSaVioR
Copy link
Contributor Author

@revans2
I just saw you also address 'storm-clojure' to refer 'storm-client' in STORM-2447. Nice work.

@revans2
Copy link
Contributor

revans2 commented Apr 2, 2017

@HeartSaVioR Yes I gave these changes a +1 because they were as good as I could expect without having storm server removed completely from the storm jar classpath. To make that happen we really had to have a different way to submit a local topology. Now that I have addressed it I was able to finish removing storm server from the classpath and I am working on removing it from all of the external packages as well.

* break down 'storm-core' into multiple artifacts
  * 'storm-client': topology and worker, play as client SDK
  * 'storm-client-misc': client SDK misc which is not added due to dependency addition
  * 'storm-server': LocalCluster and related daemons
  * 'storm-core': Clojure things and unported daemons (webapp) and tests
  * 'storm-webapp': DRPC HTTP server, Logviewer and UI will be placed here
* Supervisor will set classpath for worker to just ensure 'storm-client', not 'storm-core'
* addresse documentation
* apply the change to all modules's pom.xml
* add new modules into Travis build
* NOTE: we don't shade and relocate what we have been relocated
  * we should discuss how to overcome dependency issue
@HeartSaVioR
Copy link
Contributor Author

Just rebased.

@revans2
Copy link
Contributor

revans2 commented Apr 6, 2017

+1

@asfgit asfgit merged commit 4de339a into apache:master Apr 6, 2017
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.

3 participants