Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.0
    • Fix Version/s: 1.3.0
    • Component/s: Table API & SQL
    • Labels:
      None

      Description

      It seems that Janino is not part of the flink-table JAR file although it is a dependency in pom.xml. Users adding flink-table to Flink's lib folder because of FLINK-5227 cannot run table program due to the missing Janino dependency.

        Issue Links

          Activity

          Hide
          sunjincheng121 sunjincheng added a comment -

          Hi,Timo Walther, I glad to do this issue. Can I take this JIRA.?

          Show
          sunjincheng121 sunjincheng added a comment - Hi, Timo Walther , I glad to do this issue. Can I take this JIRA.?
          Show
          twalthr Timo Walther added a comment - Yes, of course. Thanks. This the related mail from the ML: http://apache-flink-user-mailing-list-archive.2336050.n4.nabble.com/Flink-1-2-Proper-Packaging-of-flink-table-with-SBT-td12096.html
          Hide
          sunjincheng121 sunjincheng added a comment -

          Thanks Timo Walther, I'll look at the ML, and work on this.

          Show
          sunjincheng121 sunjincheng added a comment - Thanks Timo Walther , I'll look at the ML, and work on this.
          Hide
          twalthr Timo Walther added a comment -

          Maybe this is also related with https://ci.apache.org/projects/flink/flink-docs-release-1.2/setup/building.html. "NOTE: Maven 3.3.x can build Flink, but will not properly shade away certain dependencies. Maven 3.0.3 creates the libraries properly."

          Show
          twalthr Timo Walther added a comment - Maybe this is also related with https://ci.apache.org/projects/flink/flink-docs-release-1.2/setup/building.html . "NOTE: Maven 3.3.x can build Flink, but will not properly shade away certain dependencies. Maven 3.0.3 creates the libraries properly."
          Hide
          sunjincheng121 sunjincheng added a comment - - edited

          Hi Timo Walther Recently I was working on FLIP 11 SQL OVER, and this JIRA did not keep up with. Anyhow I am sorry for that. Today I'll check this issue in my local side.
          Thanks,
          SunJincheng

          Show
          sunjincheng121 sunjincheng added a comment - - edited Hi Timo Walther Recently I was working on FLIP 11 SQL OVER, and this JIRA did not keep up with. Anyhow I am sorry for that. Today I'll check this issue in my local side. Thanks, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user sunjincheng121 opened a pull request:

          https://github.com/apache/flink/pull/3656

          FLINK-5994[table] Add Janino to flink-dist JAR file

          Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
          If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
          In addition to going through the list, please provide a meaningful description of your changes.

          • [x] General
          • The pull request references the related JIRA issue ("FLINK-5994 Add Janino to flink-dist JAR file")
          • The pull request addresses only one issue
          • Each commit in the PR has a meaningful commit message (including the JIRA id)
          • [ ] Documentation
          • Documentation has been added for new functionality
          • Old documentation affected by the pull request has been updated
          • JavaDoc for public methods has been added
          • [ ] Tests & Build
          • Functionality added by the pull request is covered by tests
          • `mvn clean verify` has been executed successfully locally or a Travis build has passed

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

          $ git pull https://github.com/sunjincheng121/flink FLINK-5994-PR

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

          https://github.com/apache/flink/pull/3656.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 #3656


          commit d8938f020bb92b05da289ba87862e76fdd83b7bb
          Author: sunjincheng121 <sunjincheng121@gmail.com>
          Date: 2017-03-31T08:31:18Z

          FLINK-5994[table] Add Janino to flink-dist JAR file


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user sunjincheng121 opened a pull request: https://github.com/apache/flink/pull/3656 FLINK-5994 [table] Add Janino to flink-dist JAR file Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration. If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide] ( http://flink.apache.org/how-to-contribute.html ). In addition to going through the list, please provide a meaningful description of your changes. [x] General The pull request references the related JIRA issue (" FLINK-5994 Add Janino to flink-dist JAR file") The pull request addresses only one issue Each commit in the PR has a meaningful commit message (including the JIRA id) [ ] Documentation Documentation has been added for new functionality Old documentation affected by the pull request has been updated JavaDoc for public methods has been added [ ] Tests & Build Functionality added by the pull request is covered by tests `mvn clean verify` has been executed successfully locally or a Travis build has passed You can merge this pull request into a Git repository by running: $ git pull https://github.com/sunjincheng121/flink FLINK-5994 -PR Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/3656.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 #3656 commit d8938f020bb92b05da289ba87862e76fdd83b7bb Author: sunjincheng121 <sunjincheng121@gmail.com> Date: 2017-03-31T08:31:18Z FLINK-5994 [table] Add Janino to flink-dist JAR file
          Hide
          sunjincheng121 sunjincheng added a comment - - edited

          Hi, Timo Walther I have opened the PR.I took the user's way to check the release package:

          hadoop:target jincheng.sunjc$ jar tf flink-table_2.10-1.3-SNAPSHOT-jar-with-dependencies.jar |grep CompileException
          javassist/CannotCompileException.class
          

          Please check if this change is that you want ?

          Best,
          SunJincheng

          Show
          sunjincheng121 sunjincheng added a comment - - edited Hi, Timo Walther I have opened the PR.I took the user's way to check the release package: hadoop:target jincheng.sunjc$ jar tf flink-table_2.10-1.3-SNAPSHOT-jar-with-dependencies.jar |grep CompileException javassist/CannotCompileException.class Please check if this change is that you want ? Best, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3656

          Hi, @twalthr @fhueske In this PR I made the following changes:
          1. Keep the `flink-table_2.10-XXX.jar` clean.
          2. Add a fat jar `flink-table_2.10-XXX-jar-with-dependencies.jar` which contains all dependent jar packages.
          3. Copy `flink-table_2.10-XXX-jar-with-dependencies.jar` to `opt/ ` folder when building flink.

          I am appreciated If you can review this PR.

          Best,
          SunJincheng

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3656 Hi, @twalthr @fhueske In this PR I made the following changes: 1. Keep the `flink-table_2.10-XXX.jar` clean. 2. Add a fat jar `flink-table_2.10-XXX-jar-with-dependencies.jar` which contains all dependent jar packages. 3. Copy `flink-table_2.10-XXX-jar-with-dependencies.jar` to `opt/ ` folder when building flink. I am appreciated If you can review this PR. Best, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3656#discussion_r109399451

          — Diff: flink-libraries/flink-table/pom.xml —
          @@ -231,6 +231,25 @@ under the License.
          </configuration>
          </plugin>

          + <!-- build a har-with-dependencies, to be included in the 'opt' build folder -->
          — End diff –

          `har` -> `jar`

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on a diff in the pull request: https://github.com/apache/flink/pull/3656#discussion_r109399451 — Diff: flink-libraries/flink-table/pom.xml — @@ -231,6 +231,25 @@ under the License. </configuration> </plugin> + <!-- build a har-with-dependencies, to be included in the 'opt' build folder --> — End diff – `har` -> `jar`
          Hide
          githubbot ASF GitHub Bot added a comment -

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

          https://github.com/apache/flink/pull/3656#discussion_r109416659

          — Diff: flink-libraries/flink-table/pom.xml —
          @@ -231,6 +231,25 @@ under the License.
          </configuration>
          </plugin>

          + <!-- build a har-with-dependencies, to be included in the 'opt' build folder -->
          + <plugin>
          + <artifactId>maven-assembly-plugin</artifactId>
          + <configuration>
          + <descriptorRefs>
          + <descriptorRef>jar-with-dependencies</descriptorRef>
          — End diff –

          The jar-with-dependencies is 86MB. This would increase the distribution from 120 MB to 200 MB.
          I looked into the jar file and it contains all dependencies of `flink-table`, including the other Flink dependencies and their transitive dependencies (netty, hadoop, avro, etc.). We need to exclude all of that and only keep those dependencies that we really need.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on a diff in the pull request: https://github.com/apache/flink/pull/3656#discussion_r109416659 — Diff: flink-libraries/flink-table/pom.xml — @@ -231,6 +231,25 @@ under the License. </configuration> </plugin> + <!-- build a har-with-dependencies, to be included in the 'opt' build folder --> + <plugin> + <artifactId>maven-assembly-plugin</artifactId> + <configuration> + <descriptorRefs> + <descriptorRef>jar-with-dependencies</descriptorRef> — End diff – The jar-with-dependencies is 86MB. This would increase the distribution from 120 MB to 200 MB. I looked into the jar file and it contains all dependencies of `flink-table`, including the other Flink dependencies and their transitive dependencies (netty, hadoop, avro, etc.). We need to exclude all of that and only keep those dependencies that we really need.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3656

          Btw. I think this PR does not address the liked JIRA issue. This PR adds the `flink-table` jar file to the `./opt` folder.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3656 Btw. I think this PR does not address the liked JIRA issue. This PR adds the `flink-table` jar file to the `./opt` folder.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3656

          Hi @fhueske In the ML: http://apache-flink-user-mailing-list-archive.2336050.n4.nabble.com/Flink-1-2-Proper-Packaging-of-flink-table-with-SBT-td12096.html user want `org/codehaus/commons/compiler/CompileException` included in flink-talbe.xxx.jar. But In my site when we build the flink-table.xx.jar, we can see that the `janino` has excluded. So, I make those changes in this PR. Then user can add `jar-with-dependencies.jar` in application class path. Do you think `janino ` should included in flink-table.xxx.jar ? or Is there something wrong in my site?
          Thanks,
          SunJincheng

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3656 Hi @fhueske In the ML: http://apache-flink-user-mailing-list-archive.2336050.n4.nabble.com/Flink-1-2-Proper-Packaging-of-flink-table-with-SBT-td12096.html user want `org/codehaus/commons/compiler/CompileException` included in flink-talbe.xxx.jar. But In my site when we build the flink-table.xx.jar, we can see that the `janino` has excluded. So, I make those changes in this PR. Then user can add `jar-with-dependencies.jar` in application class path. Do you think `janino ` should included in flink-table.xxx.jar ? or Is there something wrong in my site? Thanks, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3656

          The jar-with-dependencies should include Janino. This jar needs to include everything that is required to run flink-table, but nothing that is already included in flink-dist.jar.

          If I understand correctly, FLINK-5994 is about adding Janino to the default jar artifact which is deployed to Maven. Actually, I'm not sure if that should be done because Maven would load that dependency automatically.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3656 The jar-with-dependencies should include Janino. This jar needs to include everything that is required to run flink-table, but nothing that is already included in flink-dist.jar. If I understand correctly, FLINK-5994 is about adding Janino to the default jar artifact which is deployed to Maven. Actually, I'm not sure if that should be done because Maven would load that dependency automatically.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3656

          I will create another JIRA for this issue and assign it to you.

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3656 I will create another JIRA for this issue and assign it to you.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3656

          Here's the JIRA for this PR: https://issues.apache.org/jira/browse/FLINK-6247

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3656 Here's the JIRA for this PR: https://issues.apache.org/jira/browse/FLINK-6247
          Hide
          fhueske Fabian Hueske added a comment -

          I don't think we need to include Janino into the default flink-table artifact. If the flink-table dependency is added to a project (pom.xml), Maven (or any other build tool) will automatically fetch the required dependencies including Janino. The default jar is not the right file to add to the ./lib folder.

          I created FLINK-6247 to create a jar-with-dependencies for flink-table which should be put to the ./opt folder. From there users can copy it to ./lib to load the Table API into the default class loader.

          I propose to close this issue.

          Show
          fhueske Fabian Hueske added a comment - I don't think we need to include Janino into the default flink-table artifact. If the flink-table dependency is added to a project (pom.xml), Maven (or any other build tool) will automatically fetch the required dependencies including Janino. The default jar is not the right file to add to the ./lib folder. I created FLINK-6247 to create a jar-with-dependencies for flink-table which should be put to the ./opt folder. From there users can copy it to ./lib to load the Table API into the default class loader. I propose to close this issue.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3656

          Thanks @fhueske , I had moved the PR to FLINK-6247, and think about how to deal with this JIRA.

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3656 Thanks @fhueske , I had moved the PR to FLINK-6247 , and think about how to deal with this JIRA.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user fhueske commented on the issue:

          https://github.com/apache/flink/pull/3656

          Thanks @sunjincheng121. I think you can close this PR.
          I also proposed to close FLINK-5994, because I think Janino shouldn't be added to the default artifact but to the jar-with-dependencies instead (FLINK-6247).

          Show
          githubbot ASF GitHub Bot added a comment - Github user fhueske commented on the issue: https://github.com/apache/flink/pull/3656 Thanks @sunjincheng121. I think you can close this PR. I also proposed to close FLINK-5994 , because I think Janino shouldn't be added to the default artifact but to the jar-with-dependencies instead ( FLINK-6247 ).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3656

          Thanks,@fhueske, I also think add `Janino` to `jar-with-dependencies` can solve the user's problem which mentioned in this JIRA.That is why at the beginning I am in this PR I did not add `Janino` to the default artifact , but add `Janino` to `jar-with-dependencies` instead. I agree to both close this PR and this JIRA. What do you think? @twalthr
          Best,
          SunJincheng

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3656 Thanks,@fhueske, I also think add `Janino` to `jar-with-dependencies` can solve the user's problem which mentioned in this JIRA.That is why at the beginning I am in this PR I did not add `Janino` to the default artifact , but add `Janino` to `jar-with-dependencies` instead. I agree to both close this PR and this JIRA. What do you think? @twalthr Best, SunJincheng
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user sunjincheng121 closed the pull request at:

          https://github.com/apache/flink/pull/3656

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

          Github user sunjincheng121 commented on the issue:

          https://github.com/apache/flink/pull/3656

          Hi, @twalthr I agree with @fhueske 's suggestion, And I'll close this JIRA. tomorrow.(2017.4.9).
          Thanks,
          SunJincheng

          Show
          githubbot ASF GitHub Bot added a comment - Github user sunjincheng121 commented on the issue: https://github.com/apache/flink/pull/3656 Hi, @twalthr I agree with @fhueske 's suggestion, And I'll close this JIRA. tomorrow.(2017.4.9). Thanks, SunJincheng
          Hide
          fhueske Fabian Hueske added a comment -

          Fixed with FLINK-6247

          Show
          fhueske Fabian Hueske added a comment - Fixed with FLINK-6247

            People

            • Assignee:
              sunjincheng121 sunjincheng
              Reporter:
              twalthr Timo Walther
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development