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

[BEAM-6558] Add IWYU plugin, activate for Beam SQL, fix errors #7700

Merged
merged 2 commits into from
Feb 2, 2019

Conversation

kennknowles
Copy link
Member

Beam SQL was successfully compiling and passing tests even though it had a totally straightforward compile-time dependency on commons-codec, which it did not declare.

I had thought that Gradle made this issue a thing of the past, but it is not so. This adds the capability that we got from mvn dependency:analyze plugin. It found the known error and a couple other issues, which may or may not be spurious.

This needs to be cherrypicked for 2.10.0 as Beam SQL was nonfunctional for 2.9.0. We can advise on a workaround to simply add the necessary dependency. We also need an end-to-end test that will ensure our shipped artifact works for users.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@amaliujia
Copy link
Contributor

amaliujia commented Feb 1, 2019

If commons-codec was not declared, where did BeamSQL find it and use it to pass tests? Was that because one or more of BeamSQL's dependencies include commons-codec somehow?

@kanterov
Copy link
Member

kanterov commented Feb 1, 2019

I didn't dig deep, but I see a similar problem with ClassNotFoundException: javax.annotation.Nullable using new DefaultSchema providers that traverse class methods using reflection and check for annotations. My solution to the problem was adding extra jar to the classpath.

I'm wondering if it's the same kind of problem as we have with commons-codec in Beam SQL?

@nickpan47
Copy link

@kennknowles liked the idea to add end-to-end tests. I noticed that there are some user example project such as word-count-example that are generated as part of the build. It would be nice to add some SQL example projects their as well, and make sure the release validation process actually builds and runs those example applications. I can create a JIRA to add BeamSQL example project, if it is not there yet.

@kennknowles
Copy link
Member Author

@amaliujia yes, one of the dependencies (actually lots of them) depend on commons-codec. Since they probably depend on it in compile scope, which means it could be on the API surface, they are put on the classpath during compilation, rather than just at runtime.

@kennknowles kennknowles changed the title [BEAM-6566] Add IWYU plugin, activate for Beam SQL, fix errors [BEAM-6558] Add IWYU plugin, activate for Beam SQL, fix errors Feb 1, 2019
@kennknowles kennknowles force-pushed the sql-deps branch 4 times, most recently from a8cc8a7 to 8051be2 Compare February 1, 2019 21:24
@kennknowles
Copy link
Member Author

OK, got past legit failures. Deflaking time.

@kennknowles
Copy link
Member Author

Run java precommit

Copy link
Contributor

@swegner swegner left a comment

Choose a reason for hiding this comment

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

LGTM

@swegner swegner merged commit 271d8d0 into apache:master Feb 2, 2019
@kennknowles kennknowles deleted the sql-deps branch January 4, 2024 20:35
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.

5 participants