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.

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

Switch to Java 17 for all Dataflow Templates #2050

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Switch to Java 17 for all Dataflow Templates #2050

wants to merge 12 commits into from

Conversation

liferoad
Copy link
Contributor

@liferoad liferoad commented Dec 5, 2024 •
edited
Loading

Fixes #2051

@codecov Codecov
Copy link

codecov bot commented Dec 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 46.58%. Comparing base (9d39b07) to head (1bf3467).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...am/it/gcp/artifacts/matchers/ArtifactsSubject.java 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##               main    #2050      +/-   ##
============================================
- Coverage     46.58%   46.58%   -0.01%     
+ Complexity     3960     3950      -10     
============================================
  Files           869      867       -2     
  Lines         51599    51550      -49     
  Branches       5403     5398       -5     
============================================
- Hits          24036    24012      -24     
+ Misses        25844    25820      -24     
+ Partials       1719     1718       -1
Components Coverage Δ
spanner-templates 68.55% <ø> (-0.02%) ⬇️
spanner-import-export 65.57% <ø> (ø)
spanner-live-forward-migration 77.36% <ø> (ø)
spanner-live-reverse-replication 78.26% <ø> (-0.07%) ⬇️
spanner-bulk-migration 87.62% <ø> (ø)
Files with missing lines Coverage Δ
...gle/cloud/teleport/plugin/DockerfileGenerator.java 90.56% <ø> (ø)
...am/it/gcp/artifacts/matchers/ArtifactsSubject.java 0.00% <0.00%> (ø)

... and 7 files with indirect coverage changes

@Abacn
Copy link
Contributor

Abacn commented Dec 5, 2024

there is a single test failure from the report: https://github.com/GoogleCloudPlatform/DataflowTemplates/pull/2050/checks?check_run_id=33950822829

expected to contain hash: 81f7b7afd932b4754caaa9ba6ced7a8bcb2cbfec6857cf823e4d112125c6e939
but was                 : [org.apache.beam.it.gcp.artifacts.GcsArtifact@40673c72]
	at com.google.cloud.teleport.templates.BulkCompressorIT.baseCompress(BulkCompressorIT.java:85)
	at com.google.cloud.teleport.templates.BulkCompressorIT.testCompressGzip(BulkCompressorIT.java:55)

This is expected. The original assert was too strict, see

https://stackoverflow.com/questions/77305259/change-in-gzipoutputstream-behavior-in-java-17

@Abacn
Copy link
Contributor

Abacn commented Dec 5, 2024

Kafka PR running on https://github.com/GoogleCloudPlatform/DataflowTemplates/actions/runs/12183265029

Spanner PR running on https://github.com/GoogleCloudPlatform/DataflowTemplates/actions/runs/12183272085

@liferoad liferoad mentioned this pull request Dec 5, 2024
Open
@liferoad liferoad changed the title [DO NOT MERGE] Test Java 17 [DO NOT MERGE] Switch to Java 17 for all Dataflow Templates Dec 13, 2024
@liferoad
Copy link
Contributor Author

TODO: fix #2050 (comment)

@liferoad liferoad marked this pull request as ready for review January 9, 2025 14:54
@liferoad liferoad requested review from a team as code owners January 9, 2025 14:54
@liferoad liferoad changed the title [DO NOT MERGE] Switch to Java 17 for all Dataflow Templates Switch to Java 17 for all Dataflow Templates Jan 9, 2025
@liferoad liferoad requested a review from Abacn January 9, 2025 14:54
Abacn
Abacn approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@Abacn Abacn left a comment

Choose a reason for hiding this comment

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

Thanks! Just have a minor comment, could be follow up (so no need to push new commit that triggers all tests)

...cloud-platform/src/main/java/org/apache/beam/it/gcp/artifacts/matchers/ArtifactsSubject.java
.forEach(
artifact -> {
String calculatedHash = sha256().hashBytes(artifact.contents()).toString();
System.out.printf("Calculated Hash (no match found): {%s} \n", calculatedHash);
Copy link
Contributor

Choose a reason for hiding this comment

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

switch back to LOG.warn after debug?

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

@Polber Polber Polber requested changes

@Abacn Abacn Abacn approved these changes

@asthamohta asthamohta Awaiting requested review from asthamohta asthamohta is a code owner automatically assigned from GoogleCloudPlatform/spanner-migrations-team

@VardhanThigle VardhanThigle Awaiting requested review from VardhanThigle VardhanThigle is a code owner automatically assigned from GoogleCloudPlatform/spanner-migrations-team

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
improvement size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Use Java 17 for Dataflow Templates
3 participants
@liferoad @Abacn @Polber