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

[FLINK-4111] [table] Flink Table & SQL doesn't work in very simple example #2209

Closed
wants to merge 3 commits into from

Conversation

twalthr
Copy link
Contributor

@twalthr twalthr commented Jul 6, 2016

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.
In addition to going through the list, please provide a meaningful description of your changes.

  • General
    • The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
    • 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

This PR reworks the flink-table pom.xml. It simplifies the file, removes unnecessary dependencies and allows to build jobs with flink-table dependency.

@rmetzger @tillrohrmann Would be great if you could take a look on it.

@@ -126,14 +125,15 @@ under the License.
<plugin>
<groupId>net.alchim31.maven</groupId>
<artifactId>scala-maven-plugin</artifactId>
<version>3.1.4</version>
<version>3.2.2</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you update the plugin version?
The problem is now that we are using different plugin versions across the project.
I would suggest to remove the plugin versions from all usages of the plugin and put it into the pluginManagement section of the root-pom.

Copy link
Contributor Author

@twalthr twalthr Jul 8, 2016

Choose a reason for hiding this comment

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

I used the newest library flink-cep-scala as reference. We are already using different plugin versions. I will introduce a global variable for all projects.

@rmetzger
Copy link
Contributor

rmetzger commented Jul 7, 2016

Are all the dependencies you removed really not needed or are they needed, but present because they are pulled in transitively?
In general, it makes sense to define all needed dependencies, instead of relying on transitive dependencies.

The problem with the suggested shading is that you are not relocating calcite, which means that we have calcite in the org.apache.calcite namespace, in our flink package.
This can potentially lead to version clashes if our users want to use a different calcite version in their code (I have to admit that's unlikely.)
But for com.fasterxml.jackson this might be an issue, also for org.slf4j.

I would exclude slf4j and jackson from the shading.

Do you know why the relocation of calcite doesn't work? Do you think there is an issue with relocating class files produced by the scala compiler?

@twalthr
Copy link
Contributor Author

twalthr commented Jul 8, 2016

@rmetzger I have reworked the pom.xml again. Now the JAR is as clean as possible. All unneccessary files are filtered out. Most of the other dependencies (com.google, com.fasterxml.jackson) are relocated.

I could not relocate org.slf4j because that would lead to a warning during runtime. Relocating Calcite leads to weird Scala compiler problems, but I think no one wants to use a custom Caclite version.

I have introduced a global scala maven plugin version for all scala modules.

@rmetzger
Copy link
Contributor

I think the change is good to merge.

@twalthr
Copy link
Contributor Author

twalthr commented Jul 12, 2016

Thanks @rmetzger. I would merge it today if there are no objections.

@twalthr
Copy link
Contributor Author

twalthr commented Jul 12, 2016

Merging...

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