fix flaky test #30322

m-trieu · 2024-02-15T00:29:08Z

Removed flaky logic around waiting in tests.

Removed thread.sleep and replaced with triggers via CountDownLatch

R: @scwhittle @Abacn

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

github-actions · 2024-02-15T01:26:06Z

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

R: @damccorm added as fallback since no labels match configuration

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).

scwhittle · 2024-02-15T10:40:59Z

.../org/apache/beam/runners/dataflow/worker/windmill/client/grpc/StreamingEngineClientTest.java

+      TestGetWorkBudgetDistributor getWorkBudgetDistributor) throws InterruptedException {
+    getWorkBudgetDistributor.waitForBudgetDistribution();
+    while (isBudgetRefreshPaused.get()) {
+      // wait for budget refresh til budget refresh is unpaused.


add a little sleep to not burn cpu and slow test down

also can you improve comment on why this is necessary?
It seems we pause during updating the endpoints. But we set that to false before we trigger the refresher.

Friendly ping in case this got lost

done, removed and added sleep.

scwhittle · 2024-02-15T10:48:19Z

.../org/apache/beam/runners/dataflow/worker/windmill/client/grpc/StreamingEngineClientTest.java

    @Override
    public void distributeBudget(
        ImmutableCollection<WindmillStreamSender> streams, GetWorkBudget getWorkBudget) {
      streams.forEach(stream -> stream.adjustBudget(getWorkBudget.items(), getWorkBudget.bytes()));
+      getWorkBudgetDistributorTriggered.countDown();


does this throw if we decrement too much? if so with the schedled test it seems racy if it happens too often. Maybe could check before decrementing here since it is single-threaded distributing.

are these tests run on multiple threads in the GHA env?
are the @before and @after code blocks not being run?

it doesn't
from javadoc:

countDown public void countDown() Decrements the count of the latch, releasing all waiting threads if the count reaches zero. If the current count is greater than zero then it is decremented. If the new count is zero then all waiting threads are re-enabled for thread scheduling purposes. If the current count equals zero then nothing happens.

https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/CountDownLatch.html#countDown--

scwhittle · 2024-02-15T10:50:09Z

Do you know how to run a test 100x to verify it isn't flaky? I'm not sure if there is a gradle invocation to do that or not. Maybe @Abacn knows

Abacn · 2024-02-15T15:24:36Z

Do you know how to run a test 100x to verify it isn't flaky? I'm not sure if there is a gradle invocation to do that or not. Maybe @Abacn knows

It's unit test and the command can be this

first in https://github.com/apache/beam/blob/master/runners/google-cloud-dataflow-java/worker/build.gradle add a line

test.outputs.upToDateWhen {false}

(ref: https://stackoverflow.com/questions/29427020/how-to-run-gradle-test-when-all-tests-are-up-to-date);

then use a script like

for i in `seq 1 100`; do
./gradlew :runners:google-cloud-dataflow-java:worker:test --tests *someSpecificUnitTest*
done

and see the result.

The problem is the tests with racing condition tends to be more flaky on GitHub Action than on local machines. This may be because the GHA runner has different load each time run, which affects timing, but the local machine do not

m-trieu · 2024-02-15T16:38:32Z

Thanks! I'll run it and circle back

@Abacn is there anyway I can run it in the same environment as GitHub actions? Or simulate the different loads?

m-trieu · 2024-02-15T20:58:37Z

running this command locally

:runners:google-cloud-dataflow-java:worker:test --tests "org.apache.beam.runners.dataflow.worker.windmill.client.grpc.StreamingEngineClientTest"

m-trieu · 2024-02-15T22:08:00Z

:runners:google-cloud-dataflow-java:worker:test --tests "org.apache.beam.runners.dataflow.worker.windmill.client.grpc.StreamingEngineClientTest"

was successful @Abacn

scwhittle · 2024-02-16T09:58:56Z

.../org/apache/beam/runners/dataflow/worker/windmill/client/grpc/StreamingEngineClientTest.java

+      TestGetWorkBudgetDistributor getWorkBudgetDistributor) throws InterruptedException {
+    getWorkBudgetDistributor.waitForBudgetDistribution();
+    while (isBudgetRefreshPaused.get()) {
+      // wait for budget refresh til budget refresh is unpaused.


Friendly ping in case this got lost

scwhittle · 2024-02-16T10:00:07Z

.../org/apache/beam/runners/dataflow/worker/windmill/client/grpc/StreamingEngineClientTest.java

+
+    // Make sure we are injecting the metadata from smallest to largest.
+    workerMetadataResponses.stream()
+        .sorted(Comparator.comparingLong(WorkerMetadataResponse::getMetadataVersion).reversed())


the reversed doesn't make sense with the comment. Is it wrong or can comment be improved to make it clearer what reversing is needed?

scwhittle · 2024-02-20T09:08:21Z

.../org/apache/beam/runners/dataflow/worker/windmill/client/grpc/StreamingEngineClientTest.java

+  private void waitForWorkerMetadataToBeConsumed(
+      TestGetWorkBudgetDistributor getWorkBudgetDistributor) throws InterruptedException {
+    getWorkBudgetDistributor.waitForBudgetDistribution();
+    Thread.sleep(100);


I just meant to have a sleep in the loop checking the atomic value.
Since we removed that, the sleep is unnecessary

gotcha removed

github-actions bot added runners dataflow labels Feb 15, 2024

m-trieu mentioned this pull request Feb 15, 2024

Update windmill proto definition #30046

Merged

3 tasks

github-actions bot added the Next Action: Reviewers label Feb 15, 2024

m-trieu mentioned this pull request Feb 15, 2024

[Failing Test]: dataflow runner worker project test stuck causing Java PreCommit time out #28957

Open

16 tasks

scwhittle requested changes Feb 15, 2024

View reviewed changes

m-trieu force-pushed the mt-flaky-test branch from 7d9911a to 70ee9dd Compare February 15, 2024 20:47

scwhittle requested changes Feb 16, 2024

View reviewed changes

scwhittle requested changes Feb 20, 2024

View reviewed changes

m-trieu added 2 commits February 20, 2024 10:51

fix flaky test

d95bc54

address cl comments

572e106

m-trieu force-pushed the mt-flaky-test branch from c4f6d2c to 572e106 Compare February 20, 2024 18:52

m-trieu and others added 2 commits February 20, 2024 11:15

remove Thread.sleep() from waiting in test

f10e2d9

Fix spotless

6d5e398

scwhittle approved these changes Feb 21, 2024

View reviewed changes

scwhittle merged commit b78a3e1 into apache:master Feb 21, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix flaky test #30322

fix flaky test #30322

m-trieu commented Feb 15, 2024

github-actions bot commented Feb 15, 2024

scwhittle Feb 15, 2024

scwhittle Feb 15, 2024

scwhittle Feb 16, 2024

m-trieu Feb 20, 2024

scwhittle Feb 15, 2024

m-trieu Feb 15, 2024

m-trieu Feb 15, 2024

scwhittle commented Feb 15, 2024

Abacn commented Feb 15, 2024

m-trieu commented Feb 15, 2024

m-trieu commented Feb 15, 2024

m-trieu commented Feb 15, 2024

scwhittle Feb 16, 2024

scwhittle Feb 16, 2024

m-trieu Feb 20, 2024

scwhittle Feb 20, 2024

m-trieu Feb 20, 2024

fix flaky test #30322

fix flaky test #30322

Conversation

m-trieu commented Feb 15, 2024

R: @scwhittle @Abacn

GitHub Actions Tests Status (on master branch)

github-actions bot commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scwhittle commented Feb 15, 2024

Abacn commented Feb 15, 2024

m-trieu commented Feb 15, 2024

m-trieu commented Feb 15, 2024

m-trieu commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment