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

Makes timeout for owner lookup in StaticQueryExecutor and rebalancin… #3856

Merged

Conversation

AlanConfluent
Copy link
Member

…g in KsStateStore configurable

Description

Fixes #3573

Both timeouts in StaticQueryExecutor and KsStateStore have been factored out to a configuration value that can be updated.

Testing done

Ran all unit tests by mvn clean install checkstyle:check integration-test

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@AlanConfluent AlanConfluent requested a review from a team as a code owner November 14, 2019 22:35
@ghost
Copy link

ghost commented Nov 14, 2019

It looks like @AlanConfluent hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

Copy link
Member

@stevenpyzhang stevenpyzhang left a comment

Choose a reason for hiding this comment

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

LGTM, there's some leftover constants that should be removed, maybe rename a config, and one typo (I think). Also, this is my first time looking into the KsStateStoreTest.java, @big-andy-coates what is this line doing if the timeout variable isn't being used in the test?

  @Rule
  public final Timeout timeout = Timeout.seconds(10);

@stevenpyzhang stevenpyzhang requested a review from a team November 14, 2019 22:56
@AlanConfluent
Copy link
Member Author

clabot:check

@stevenpyzhang stevenpyzhang requested a review from a team November 14, 2019 23:14
@AlanConfluent AlanConfluent force-pushed the configurable_timeouts branch 3 times, most recently from 3313875 to 4f448b7 Compare November 15, 2019 01:00
Copy link
Contributor

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

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

1 comment on naming. Let me know what you think.

@AlanConfluent AlanConfluent force-pushed the configurable_timeouts branch 3 times, most recently from 0ec8b8f to c9d3a5e Compare November 15, 2019 19:20
Copy link
Contributor

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

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

Just 1 more thing, based on the issue you filed. You can make the call.
LGTM otherwise.

@AlanConfluent AlanConfluent merged commit 39245c6 into confluentinc:master Nov 16, 2019
@big-andy-coates
Copy link
Contributor

@big-andy-coates what is this line doing if the timeout variable isn't being used in the test?

  @Rule
  public final Timeout timeout = Timeout.seconds(10);

That will cause the test to fail after 10 seconds if it hasn't completed.

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.

Pull Queries: Make timeouts configurable
5 participants