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

Do not run spanner-pr tests in Java PR #1826

Merged
merged 2 commits into from
Sep 4, 2024
Merged

Do not run spanner-pr tests in Java PR #1826

merged 2 commits into from
Sep 4, 2024

Conversation

Abacn
Copy link
Contributor

@Abacn Abacn commented Aug 29, 2024

spanner-pr tests have been flaky, mostly due to different kind of resource exhausted. This PR moves them out of. the common java pr GHA.

Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.19%. Comparing base (3b59345) to head (11daeb1).
Report is 8 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1826      +/-   ##
============================================
+ Coverage     43.08%   43.19%   +0.10%     
- Complexity     3774     3789      +15     
============================================
  Files           829      829              
  Lines         48418    48425       +7     
  Branches       5193     5194       +1     
============================================
+ Hits          20862    20915      +53     
+ Misses        25877    25832      -45     
+ Partials       1679     1678       -1     
Components Coverage Δ
spanner-templates 63.63% <ø> (+0.31%) ⬆️
spanner-import-export 63.81% <ø> (-0.07%) ⬇️
spanner-live-forward-migration 75.11% <ø> (+0.05%) ⬆️
spanner-live-reverse-replication 68.93% <ø> (+1.30%) ⬆️
spanner-bulk-migration 84.13% <ø> (+0.03%) ⬆️

see 9 files with indirect coverage changes

@Abacn
Copy link
Contributor Author

Abacn commented Aug 29, 2024

java pr instead run on dispatch_workflow trigger: https://github.com/GoogleCloudPlatform/DataflowTemplates/actions/runs/10617855255/job/29432175100 because pr trigger won't pick up changes in github action yaml

@Abacn
Copy link
Contributor Author

Abacn commented Aug 29, 2024

Verified there are 6 modules no longer build on Java PR workflow. Previously,

Building plaintext-logging 1.0-SNAPSHOT                           [5/74]

now

Building spanner-common 1.0-SNAPSHOT                             [26/68]

@Abacn Abacn marked this pull request as ready for review August 29, 2024 16:03
@Abacn
Copy link
Contributor Author

Abacn commented Aug 29, 2024

R: @Polber @damccorm

cc: @darshan-sj @aksharauke

Copy link
Contributor

@Polber Polber left a comment

Choose a reason for hiding this comment

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

My concern is that changes that would normally affect all templates won't be tested against all templates.

For example, SPANNER includes v2/sourcedb-to-spanner/. This module depends on v2/common. Say someone modifies the CommonTemplateJvmInitializer in v2/common to fix something in one of the JDBC templates. The Sourcedb_to_Spanner_Flex template also uses this class. So, even though there is no direct dependency on common, spanner is still affected when there are changes in it.

Or another simple example - changes to it/ will affect all templates, so it should probably be tested against all templates

@Abacn
Copy link
Contributor Author

Abacn commented Aug 30, 2024

Changes affects all templates won't necessary to test against all templates. If it is in common module, it should also affects non-spanner module. If a change only affects spanner module, the code involved shouldn't be in the common module at the first place.

If there are code path sounds necessary to exercise spanner tests, we can add those to trigger path of spanner-pr.yml

Currently spanner integration tests are running twice if a PR touches trigger files or both java-pr and spanner-pr. This is highly ineffective. The current situation is a big workflow running all integration test is not sustainable. It's taking increasingly long, and spanner tests consistently becomes trouble maker. It's blocking major template maintenance work. Please consider split the tests as necessary movement as of review.

@Abacn
Copy link
Contributor Author

Abacn commented Aug 30, 2024

This is more like a outage fix change, to fix the already permared java-pr workflow. The spanner integration tests has various issues, which will be considered later by corresponding code owners.

@Abacn
Copy link
Contributor Author

Abacn commented Aug 30, 2024

There is also plan to split Kafka tests - #1800 (comment) the cicd/ change introduced in this PR will also help that

@Polber
Copy link
Contributor

Polber commented Aug 31, 2024

Changes affects all templates won't necessary to test against all templates. If it is in common module, it should also affects non-spanner module. If a change only affects spanner module, the code involved shouldn't be in the common module at the first place.

If there are code path sounds necessary to exercise spanner tests, we can add those to trigger path of spanner-pr.yml

Currently spanner integration tests are running twice if a PR touches trigger files or both java-pr and spanner-pr. This is highly ineffective. The current situation is a big workflow running all integration test is not sustainable. It's taking increasingly long, and spanner tests consistently becomes trouble maker. It's blocking major template maintenance work. Please consider split the tests as necessary movement as of review.

@Abacn I think splitting makes sense, I was just pointing out it could lead to unexpected build failures for non-default modules (i.e. spanner, and soon kafka)

For spanner at least, maybe we should just inform them of the change and point out the following modules are transitive dependencies of the already listed modules:

v2/common
v2/datastream-common

v2/spanner-common
v2/spanner-migrations-sdk
v2/spanner-custom-shard

it/google-cloud-platform
it/conditions

Adding these to the spanner module list could help prevent unexpected failures, and I don't think there are any IT's that would add slowdown to the existing workflow (just some unit tests) - this could also help find potential issues before the release which does run ALL tests

Polber
Polber previously approved these changes Aug 31, 2024
@Abacn
Copy link
Contributor Author

Abacn commented Sep 3, 2024

Thanks, added these paths to spanner-pr trigger path

@Abacn
Copy link
Contributor Author

Abacn commented Sep 3, 2024

one integration test failing, not related to this PR:

SpannerChangeStreamsToBigQueryIT.testSpannerChangeStreamsToBigQueryFloatColumns

org.apache.beam.sdk.extensions.gcp.util.RetryHttpRequestInitializer - Request failed with code 412, performed 0 retries due to IOExceptions, performed 0 retries due to unsuccessful status codes, HTTP framework says request can be retried, (caller responsible for retrying): https://storage.googleapis.com/upload/storage/v1/b/dataflow-staging-us-central1-269744978479/o?ifGenerationMatch=0&name=staging/common-1.0-SNAPSHOT-i81N2xsQTnm2tfk1XSXEZrxZwuvedBIdmffiqGZdnco.jar&uploadType=resumable&upload_id=AD-8ljt6Dxbk175_A5TsvTt6BOYXTMSxG7deYbHhQUcPu8eV9MYYhsqQAIqqm4PXbstfTPWXdTE7WKIjBvgFYBfWm3InVP_MoE-itu47IyVvWo8T. 

...

Exception: java.lang.NoSuchMethodError thrown from the UncaughtExceptionHandler in thread "main"

same test on spanner-pr passed. Likely some flakiness then failing the test, and immediately triggering an unknown dependency version issue when template launching is failing

@Abacn
Copy link
Contributor Author

Abacn commented Sep 4, 2024

opened #1836 for the flaky test seen

Copy link
Contributor

@shunping shunping left a comment

Choose a reason for hiding this comment

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

LGTM

@Abacn Abacn merged commit feff570 into main Sep 4, 2024
24 of 26 checks passed
@Abacn Abacn deleted the splitspanner branch September 4, 2024 00:40
asthamohta pushed a commit to asthamohta/DataflowTemplates that referenced this pull request Sep 15, 2024
* split java-pr

* add common paths to spanner-pr trigger path
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants