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

Fix StringSet tests on portable runners #31999

Merged
merged 4 commits into from
Jul 31, 2024
Merged

Conversation

Abacn
Copy link
Contributor

@Abacn Abacn commented Jul 26, 2024

Reverts #31818

@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024

after #31838 now portable runners can see stringset metrics, however, the result is different from non-portable ones and still failing the assert

Previous:

java.lang.AssertionError: 
Expected: iterable with items [
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sources", step="MyStep1", attempted=<StringSetResult{stringSet=[gcs]}>},
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sinks", step="MyStep2", attempted=<StringSetResult{stringSet=[kafka, bq]}>},
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sideinputs", step="MyStep1", attempted=<StringSetResult{stringSet=[bigtable, spanner]}>},
     MetricResult{inNamespace="org.apache.beam.sdk.metrics.MetricsTest", name="sideinputs", step="MyStep2", attempted=<StringSetResult{stringSet=[sql, bigtable]}>}
] in any order
     but: no item matches: ... in []

now:

but: not matched: <
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sources, committedOrNull=null, attempted=StringSetResult{stringSet=[gcs]}}>

checked that metrics.getStringSets() now returns

[
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sinks, committedOrNull=null, attempted=StringSetResult{stringSet=[kafka, bq]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sources, committedOrNull=null, attempted=StringSetResult{stringSet=[gcs]}},
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[bigtable, sql]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[spanner, bigtable]}},
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sinks, committedOrNull=null, attempted=StringSetResult{stringSet=[kafka, bq]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sources, committedOrNull=null, attempted=StringSetResult{stringSet=[gcs]}},
MetricResult{key=MyStep2:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[bigtable, sql]}},
MetricResult{key=MyStep1-ParMultiDo-Anonymous-:org.apache.beam.sdk.metrics.MetricsTest:sideinputs, committedOrNull=null, attempted=StringSetResult{stringSet=[spanner, bigtable]}}
]

the problem is each result duplicated one more time

@github-actions github-actions bot added the java label Jul 26, 2024
@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024

Java PVR Spark Batch two tests failed: testValueStateCoderInferenceFromInputCoder testValueStateCoderInference unrelated to the change

Java ULR stringset test failed, still need to exclude it

@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024

Dataflow v2 test Still failing:

org.apache.beam.sdk.metrics.MetricsTest$AttemptedMetricTests > testAttemptedStringSetMetrics FAILED
    java.lang.AssertionError at MetricsTest.java:417
org.apache.beam.sdk.metrics.MetricsTest$CommittedMetricTests > testCommittedStringSetMetrics FAILED
    java.lang.AssertionError at MetricsTest.java:288

will investigate further

Other portable runner tests (Flink/Spark) now passing

@Abacn Abacn marked this pull request as ready for review July 26, 2024 20:48
@Abacn Abacn force-pushed the revert-31818-excludestringset branch from 4f83c8c to 03b8b00 Compare July 26, 2024 20:50
@Abacn Abacn force-pushed the revert-31818-excludestringset branch from 03b8b00 to 03d1008 Compare July 26, 2024 20:52
@github-actions github-actions bot removed the build label Jul 26, 2024
@Abacn Abacn changed the title Revert "Exclude StringSet tests from portable runners and Dataflow LegacyRunner" Fix StringSet tests on portable runners Jul 26, 2024
@Abacn
Copy link
Contributor Author

Abacn commented Jul 26, 2024

Ready for review. Latest commit reverted trigger file change. See test results of f302fb1

R: @rohitsinha54

Copy link
Contributor

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @kennknowles for label java.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@Abacn
Copy link
Contributor Author

Abacn commented Jul 29, 2024

R: @robertwb @rohitsinha54

Copy link
Contributor

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment assign set of reviewers

@Abacn Abacn merged commit eec2068 into master Jul 31, 2024
29 checks passed
@Abacn Abacn deleted the revert-31818-excludestringset branch July 31, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants