Skip to content

Fix race in BaseNodeRoleWatcher tests#16064

Merged
gianm merged 2 commits intoapache:masterfrom
AmatyaAvadhanula:followup_15726
Mar 7, 2024
Merged

Fix race in BaseNodeRoleWatcher tests#16064
gianm merged 2 commits intoapache:masterfrom
AmatyaAvadhanula:followup_15726

Conversation

@AmatyaAvadhanula
Copy link
Contributor

Follow up PR to #15726
Addresses the comments:

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@AmatyaAvadhanula AmatyaAvadhanula requested a review from gianm March 6, 2024 17:58
}
}

static void scheduleTimeout(
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, but it'd be nicer as a non-static method, so the caller doesn't need to pass in the executor. The watcher has the listenerExecutor already, so as a non-static this would be:

void scheduleTimeout(long timeoutSeconds)
{
    listenerExecutor.schedule(
        this::cacheInitializedTimedOut,
        timeout,
        TimeUnit.SECONDS
    );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

TimeUnit.SECONDS
);
BaseNodeRoleWatcher nodeRoleWatcher = new BaseNodeRoleWatcher(listenerExecutor, nodeRole);
scheduleTimeout(nodeRoleWatcher, listenerExecutor, DEFAULT_TIMEOUT_SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

See other comment about making scheduleTimeout an instance method; then this would simply be nodeRoleWatcher.scheduleTimeout(DEFAULT_TIMEOUT_SECONDS);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

assertListener(listener1, false, Collections.emptyList(), Collections.emptyList());

Assert.assertTrue(listener1.ready.await(1500, TimeUnit.MILLISECONDS));
BaseNodeRoleWatcher.scheduleTimeout(nodeRoleWatcher, exec, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks much nicer, thank you!

@AmatyaAvadhanula AmatyaAvadhanula requested a review from gianm March 7, 2024 05:50
@gianm
Copy link
Contributor

gianm commented Mar 7, 2024

thank you @AmatyaAvadhanula !

@gianm gianm merged commit 5871b81 into apache:master Mar 7, 2024
@gianm gianm added this to the 29.0.1 milestone Mar 7, 2024
@kfaraz kfaraz deleted the followup_15726 branch March 8, 2024 02:22
AmatyaAvadhanula added a commit to AmatyaAvadhanula/druid that referenced this pull request Mar 8, 2024
* Fix race in BaseNodeRoleWatcher tests

* Make non static
AmatyaAvadhanula added a commit that referenced this pull request Mar 8, 2024
* Fix race in BaseNodeRoleWatcher tests

* Make non static
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants