Uploaded image for project: 'Apache Storm'
  1. Apache Storm
  2. STORM-1202

Migrate APIs to org.apache.storm, but try to provide backwards compatability as a bridge

    Details

    • Type: New Feature
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0.0
    • Component/s: None
    • Labels:
      None

      Description

      We want to move the storm classpaths to org.apache.storm wherever possible, but we also want to provide backwards compatibility for user facing APIs whenever possible.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user asfgit closed the pull request at:

          https://github.com/apache/storm/pull/889

          Show
          githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/storm/pull/889
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user revans2 commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-170088345

          @d2r done I updated the comment in Config.

          Show
          githubbot ASF GitHub Bot added a comment - Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-170088345 @d2r done I updated the comment in Config.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user d2r commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-170073038

          • 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

          Show
          githubbot ASF GitHub Bot added a comment - Github user d2r commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-170073038 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
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user revans2 commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-169780775

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-169780775 I just filed http://dev.clojure.org/jira/browse/CLJ-1876 for the issue I was seeing with the require.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user revans2 commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-169775907

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-169775907 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user d2r commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-169690819

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user d2r commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-169690819 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user d2r commented on a diff in the pull request:

          https://github.com/apache/storm/pull/889#discussion_r49078953

          — Diff: storm-rename-hack/pom.xml —
          @@ -0,0 +1,107 @@
          +<?xml version="1.0" encoding="UTF-8"?>
          +<!--
          + Licensed to the Apache Software Foundation (ASF) under one or more
          + contributor license agreements. See the NOTICE file distributed with
          + this work for additional information regarding copyright ownership.
          + The ASF licenses this file to You under the Apache License, Version 2.0
          + (the "License"); you may not use this file except in compliance with
          + the License. You may obtain a copy of the License at
          +
          + http://www.apache.org/licenses/LICENSE-2.0
          +
          + Unless required by applicable law or agreed to in writing, software
          + distributed under the License is distributed on an "AS IS" BASIS,
          + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
          + See the License for the specific language governing permissions and
          + limitations under the License.
          +-->
          +<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
          + <modelVersion>4.0.0</modelVersion>
          + <parent>
          + <artifactId>storm</artifactId>
          + <groupId>org.apache.storm</groupId>
          + <version>0.11.0-SNAPSHOT</version>
          + <relativePath>../pom.xml</relativePath>
          + </parent>
          +
          + <groupId>org.apache.storm</groupId>
          + <artifactId>storm-rename-hack</artifactId>
          + <packaging>jar</packaging>
          +
          + <name>storm-rename-hack</name>
          +
          + <properties>
          + <mavenVersion>3.0</mavenVersion>
          + <asmVersion>5.0.2</asmVersion>
          + </properties>
          +
          + <dependencies>
          + <dependency>
          + <groupId>org.apache.storm</groupId>
          + <artifactId>storm-core</artifactId>
          + <version>$

          {project.version}

          </version>
          + <scope>provided</scope>
          + </dependency>
          + <dependency>
          + <groupId>com.google.guava</groupId>
          + <artifactId>guava</artifactId>
          + </dependency>
          + <dependency>
          + <groupId>org.ow2.asm</groupId>
          + <artifactId>asm</artifactId>
          + <version>$

          {asmVersion}</version>
          + </dependency>
          + <dependency>
          + <groupId>org.ow2.asm</groupId>
          + <artifactId>asm-commons</artifactId>
          + <version>${asmVersion}

          </version>
          + </dependency>
          + </dependencies>
          — End diff –

          nit: check indentation

          Show
          githubbot ASF GitHub Bot added a comment - Github user d2r commented on a diff in the pull request: https://github.com/apache/storm/pull/889#discussion_r49078953 — Diff: storm-rename-hack/pom.xml — @@ -0,0 +1,107 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd "> + <modelVersion>4.0.0</modelVersion> + <parent> + <artifactId>storm</artifactId> + <groupId>org.apache.storm</groupId> + <version>0.11.0-SNAPSHOT</version> + <relativePath>../pom.xml</relativePath> + </parent> + + <groupId>org.apache.storm</groupId> + <artifactId>storm-rename-hack</artifactId> + <packaging>jar</packaging> + + <name>storm-rename-hack</name> + + <properties> + <mavenVersion>3.0</mavenVersion> + <asmVersion>5.0.2</asmVersion> + </properties> + + <dependencies> + <dependency> + <groupId>org.apache.storm</groupId> + <artifactId>storm-core</artifactId> + <version>$ {project.version} </version> + <scope>provided</scope> + </dependency> + <dependency> + <groupId>com.google.guava</groupId> + <artifactId>guava</artifactId> + </dependency> + <dependency> + <groupId>org.ow2.asm</groupId> + <artifactId>asm</artifactId> + <version>$ {asmVersion}</version> + </dependency> + <dependency> + <groupId>org.ow2.asm</groupId> + <artifactId>asm-commons</artifactId> + <version>${asmVersion} </version> + </dependency> + </dependencies> — End diff – nit: check indentation
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user revans2 commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-163754893

          The test failures appear to be unrelated.

          Show
          githubbot ASF GitHub Bot added a comment - Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-163754893 The test failures appear to be unrelated.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user kishorvpatil commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-158100747

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user kishorvpatil commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-158100747 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user revans2 commented on the pull request:

          https://github.com/apache/storm/pull/889#issuecomment-157487487

          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.

          Show
          githubbot ASF GitHub Bot added a comment - Github user revans2 commented on the pull request: https://github.com/apache/storm/pull/889#issuecomment-157487487 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.
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user revans2 opened a pull request:

          https://github.com/apache/storm/pull/889

          STORM-1202: Migrate APIs to org.apache.storm, but try to provide some form of backwards compatability

          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).

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/revans2/incubator-storm STORM-1202

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/storm/pull/889.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #889


          commit dab5b0de51a6c83562d18ab8d823f77ff5c4d757
          Author: Robert (Bobby) Evans <evans@yahoo-inc.com>
          Date: 2015-11-13T19:42:13Z

          STORM-1202: Migrate APIs to org.apache.storm, but try to provide some form of backwards compatability


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user revans2 opened a pull request: https://github.com/apache/storm/pull/889 STORM-1202 : Migrate APIs to org.apache.storm, but try to provide some form of backwards compatability 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). You can merge this pull request into a Git repository by running: $ git pull https://github.com/revans2/incubator-storm STORM-1202 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/storm/pull/889.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #889 commit dab5b0de51a6c83562d18ab8d823f77ff5c4d757 Author: Robert (Bobby) Evans <evans@yahoo-inc.com> Date: 2015-11-13T19:42:13Z STORM-1202 : Migrate APIs to org.apache.storm, but try to provide some form of backwards compatability
          Hide
          revans2 Robert Joseph Evans added a comment -

          Yes this is not going to work even with an agent, so the best we can do is to support a new client submitting an old jar, but not an old client submitting an old jar.

          Show
          revans2 Robert Joseph Evans added a comment - Yes this is not going to work even with an agent, so the best we can do is to support a new client submitting an old jar, but not an old client submitting an old jar.
          Hide
          revans2 Robert Joseph Evans added a comment -

          My plan is not working as I had hoped. New clients submitting both old and new jars work well, but the old client submitting an old jar results in serialization errors in the worker. The reason is that the calculated serial version changes between the two. Even if I manipulate/add a serialVersionUID to match technically these are going to be incompatible changes, as officially the type of some fields/parent classes will have changed. Perhaps this is the best that we can do.

          Show
          revans2 Robert Joseph Evans added a comment - My plan is not working as I had hoped. New clients submitting both old and new jars work well, but the old client submitting an old jar results in serialization errors in the worker. The reason is that the calculated serial version changes between the two. Even if I manipulate/add a serialVersionUID to match technically these are going to be incompatible changes, as officially the type of some fields/parent classes will have changed. Perhaps this is the best that we can do.
          Hide
          revans2 Robert Joseph Evans added a comment -

          OK so a shim layer is not going to work. To make it happen I would have to modify the generated thrift code, which really is a non-starter, but thinking about this I think I have a way to still do it. I should be able to borrow code from the maven shade plugin, and shade the submitted jar to translate backtype.storm -> org.apache.storm and storm.trident -> org.apache.storm.trident. This code would exist in a separate jar and would be a plugin that could be configured on for those that want it, and off for those that don't. That way we have a way to support backwards compatibility but can be turned off when you know everyone has upgraded.

          Show
          revans2 Robert Joseph Evans added a comment - OK so a shim layer is not going to work. To make it happen I would have to modify the generated thrift code, which really is a non-starter, but thinking about this I think I have a way to still do it. I should be able to borrow code from the maven shade plugin, and shade the submitted jar to translate backtype.storm -> org.apache.storm and storm.trident -> org.apache.storm.trident. This code would exist in a separate jar and would be a plugin that could be configured on for those that want it, and off for those that don't. That way we have a way to support backwards compatibility but can be turned off when you know everyone has upgraded.

            People

            • Assignee:
              revans2 Robert Joseph Evans
              Reporter:
              revans2 Robert Joseph Evans
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development