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: Don't wait for streams thread to be in running state #4908

Merged
merged 3 commits into from
Mar 27, 2020

Conversation

vpapavas
Copy link
Member

Description

When forwarding to standby is enabled as part of HA, we were still waiting for streams thread to reach running state. I moved the waiting to happen only when forwarding to standby is not enabled.

Testing done

Confirmed it works as expected via experiments on EC2 cluster

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 #")

@vpapavas vpapavas requested a review from a team as a code owner March 26, 2020 22:37
@vinothchandar vinothchandar self-assigned this Mar 26, 2020
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.

Code changes themselves LGTM. approving to unblock.

couple comments on test

@@ -96,43 +82,21 @@ public void shouldThrowNPEs() {
new NullPointerTester()
.setDefault(KafkaStreams.class, kafkaStreams)
.setDefault(LogicalSchema.class, SCHEMA)
.setDefault(Supplier.class, clock)
.setDefault(KsqlConfig.class, ksqlConfig)
.testConstructors(KsStateStore.class, Visibility.PACKAGE);
}

@Test
public void shouldAwaitRunning() {
Copy link
Contributor

Choose a reason for hiding this comment

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

fix test name?


// When:
store.store(QueryableStoreTypes.sessionStore());
}

@Test
public void shouldGetStoreOnceRunning() {
// Given:
when(kafkaStreams.state()).thenReturn(State.RUNNING);
Copy link
Contributor

Choose a reason for hiding this comment

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

can't understand this change fully

@vpapavas vpapavas merged commit ceeead2 into confluentinc:master Mar 27, 2020
spena pushed a commit that referenced this pull request Mar 27, 2020
vpapavas added a commit that referenced this pull request Mar 31, 2020
@vpapavas vpapavas deleted the pull-await branch January 26, 2022 11:06
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