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-1202: Migrate APIs to org.apache.storm, but try to provide some form of backwards compatability #889

Merged
merged 12 commits into from Jan 11, 2016

Conversation

revans2
Copy link
Contributor

@revans2 revans2 commented Nov 17, 2015

This is not a typical pull request, because changing the packages is a huge task, and keeping it upmerged while this is reviewed and other patches goes in is a huge task, and error prone.

Instead the actual changes to the package/namespace is done by running

./dev-tools/move_packages.sh ./

you can wipe all of the changes clean again (Including any modifications on your branch that you have not checked in) by running
./dev-tools/cleanup.sh ./

once the changes are approved/committed these two scripts should be removed.

The other code is intended to provide a single executable that can shade a user jar so instead of using backtype.storm and storm.trident it uses org.apache.storm and org.apache.storm.trident.

These are off by default by can be enabled by adding

client.jartransformer.class: "org.apache.storm.hack.StormShadeTransformer"

to storm.yaml. I would expect all of the code related to this to be removed when we go to the 2.0 release.

I have tested this by running a 0.10.0-SNAPSHOT storm starter topologies against a cluster running this patch (along with the move_package.sh executed).

@revans2
Copy link
Contributor Author

revans2 commented Nov 17, 2015

Because this is a non-backwards compatible change, and the mailing lists have discussed moving the version numbers over to 1.0.0-SNAPSHOT I am happy to make that change here too, but I wanted to hold off and get feedback from others before doing so.

@kishorvpatil
Copy link
Contributor

I am a +1. I like this backward compatibility feature for smooth migration to package renaming. The changes look good to me. I ran some tests with topology jar compiled on 0.10.x and it works as expected.

@revans2
Copy link
Contributor Author

revans2 commented Dec 10, 2015

The test failures appear to be unrelated.

<artifactId>asm-commons</artifactId>
<version>${asmVersion}</version>
</dependency>
</dependencies>
Copy link

Choose a reason for hiding this comment

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

nit: check indentation

@d2r
Copy link

d2r commented Jan 7, 2016

Looks OK to me overall.

It would be nice to have a follow-on Jira Issue or Sub-Task to remove/revert the changes we need to bootstrap the testing for this (dev-tools, etc.).

I have tested this by running a 0.10.0-SNAPSHOT storm starter topologies against a cluster running this patch (along with the move_package.sh executed).

We found that with word_count, the .clj source file had been removed from the transformed jar, but we expected that this .clj would be present, but transformed.

@revans2
Copy link
Contributor Author

revans2 commented Jan 7, 2016

OK I finally tracked down some intermittent failures I was seeing when running the clojure word count example. It looks like there is an issue in clojure where how we are doing the require does not seem to be thread safe.

We should be good to go now if @d2r you want to take a final look. The previous failure is not related and is one of the random storm-kafka failures.

@revans2
Copy link
Contributor Author

revans2 commented Jan 7, 2016

I just filed http://dev.clojure.org/jira/browse/CLJ-1876 for the issue I was seeing with the require.

@d2r
Copy link

d2r commented Jan 8, 2016

  • Downloaded 0.11 source & pulled in this branch
  • ran the move_packages.sh, build and unpacked the tarball
  • launched daemons from the unpacked 0.11+hack build
  • downloaded 0.10 release tarball and unpacked it separately
  • Used the 0.11+hack bin/storm to launch the 0.10 storm.starter.clj.word_count
    • -> Client failed because of missing StormSubmitter package (good/expected)
  • Did the same adding -c client.jartransformer.class=org.apache.storm.hack.StormShadeTransformer
    • -> topology submitted and ran OK
    • Checked the stormjar.jar in the supervisor's stormdist directory and looked at the word_count.clj source file inside it, and the source had been rewritten from backtype.storm.... to org.apache.storm...

The one suggestion is to add to Config what the hack implementation class is (the intended value of the config) for the use case. Otherwise users might need to hunt around to find the class we provide.

Once that's added, +1

@revans2
Copy link
Contributor Author

revans2 commented Jan 8, 2016

@d2r done I updated the comment in Config.

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