Skip to content

dataflow: avoid importing log4j at all #6599

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 21, 2021
Merged

dataflow: avoid importing log4j at all #6599

merged 7 commits into from
Dec 21, 2021

Conversation

davidcavazos
Copy link
Contributor

Remove any direct dependencies of log4j and use slf4j instead, as recommended by the documentation, and update to Beam 2.34 which doesn't depend on log4j.

Confirmed with mvn dependency:tree | grep log4j-api that there are no direct or indirect dependency instances of log4j on any sample by doing this.

  • I have followed Sample Format Guide
  • pom.xml parent set to latest shared-configuration
  • Appropriate changes to README are included in PR
  • API's need to be enabled to test (tell us)
  • Environment Variables need to be set (ask us to set them)
  • Tests pass: mvn clean verify required
  • Lint passes: mvn -P lint checkstyle:check required
  • Static Analysis: mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
  • Please merge this PR for me once it is approved.

@davidcavazos davidcavazos requested review from yoshi-approver and a team as code owners December 21, 2021 20:10
@product-auto-label product-auto-label bot added api: dataflow Issues related to the Dataflow API. samples Issues that are directly related to samples. labels Dec 21, 2021
@davidcavazos
Copy link
Contributor Author

Hopefully fixes #5875

@anguillanneuf anguillanneuf self-requested a review December 21, 2021 21:58
@anguillanneuf
Copy link
Member

anguillanneuf commented Dec 21, 2021 •
edited
Loading

You may need to drop the beam version for some of these samples for tests to pass. But raise the version to ≥ 2.32.0. See #5875 (comment)

Reorder imports too.

@davidcavazos
Copy link
Contributor Author

Fixed lint issues. For the spanner sample, I'm trying Beam 2.32, if that passes I'll try 2.33 since we know something's broken with 2.34.

@davidcavazos
Copy link
Contributor Author

I noticed that the Spanner sample was using Java 8, which ends its support in 3 months. Updating to Java 11, maybe that'll fix the failing tests as well.

@davidcavazos davidcavazos merged commit 571cf0f into GoogleCloudPlatform:main Dec 21, 2021
@davidcavazos davidcavazos deleted the dataflow-log4j branch December 21, 2021 23:28
@davidcavazos
Copy link
Contributor Author

Java 11 didn't fix the tests, but downgrading to Beam 2.33 works for now. We still need to investigate why Beam 2.34 breaks SpannerIO.

cc: @jsimonweb @skuruppu @ansh0l -- tests for the SpannerIO sample with Beam 2.34 are still a blocker to upgrade versions (#5875)

@ansh0l
Copy link
Contributor

ansh0l commented Dec 27, 2021

@davidcavazos I will investigate separately on beam 2.34 issue. I'm assuming that for now, things are working with beam 2.33 in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesv lesv lesv approved these changes

@anguillanneuf anguillanneuf anguillanneuf approved these changes

@yoshi-approver yoshi-approver Awaiting requested review from yoshi-approver yoshi-approver is a code owner

Assignees

@averikitsch averikitsch

Labels
api: dataflow Issues related to the Dataflow API. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidcavazos @anguillanneuf @ansh0l @lesv @averikitsch