You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/07/26 09:27:47 UTC
[GitHub] [kafka] patrickstuedi opened a new pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
patrickstuedi opened a new pull request #11129:
URL: https://github.com/apache/kafka/pull/11129
This PR fixes a bug in StoreQueryIntegrationTest::shouldQueryOnlyActivePartitionStoresByDefault that causes the test to fail in the case of a client rebalancing. The changes in this PR make sure the test keeps re-trying after a rebalancing operation, instead of failing.
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrickstuedi commented on a change in pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
patrickstuedi commented on a change in pull request #11129:
URL: https://github.com/apache/kafka/pull/11129#discussion_r678945688
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StoreQueryIntegrationTest.java
##########
@@ -152,11 +150,17 @@ public void shouldQueryOnlyActivePartitionStoresByDefault() throws Exception {
}
return true;
} catch (final InvalidStateStoreException exception) {
- assertThat(
- exception.getMessage(),
- containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING")
- );
- LOG.info("Streams wasn't running. Will try again.");
+ final String message = exception.getMessage();
+ final boolean exceptionNotRunning = message.startsWith("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING");
+ if (exceptionNotRunning) {
+ LOG.info("Streams wasn't running. Will try again.");
+ }
+ final boolean exceptionRebalanced = message.startsWith("The state store, source-table, may have migrated to another instance");
+ if (exceptionRebalanced) {
+ LOG.info("Rebalancing happened. Will try again.");
+ }
+ final boolean expectedException = exceptionNotRunning || exceptionRebalanced;
+ assertThat(expectedException, is(true));
Review comment:
Great, I was looking for something like oneOf, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei merged pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
vvcephei merged pull request #11129:
URL: https://github.com/apache/kafka/pull/11129
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #11129: MINOR: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #11129:
URL: https://github.com/apache/kafka/pull/11129#issuecomment-889472160
Oy. I didn't notice the commit title was wrongly formatted before merging. Fixed in the PR title anyway.
Thanks for this contribution, @patrickstuedi ! And thanks for the review, @showuon !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on a change in pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #11129:
URL: https://github.com/apache/kafka/pull/11129#discussion_r678478897
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StoreQueryIntegrationTest.java
##########
@@ -152,11 +150,17 @@ public void shouldQueryOnlyActivePartitionStoresByDefault() throws Exception {
}
return true;
} catch (final InvalidStateStoreException exception) {
- assertThat(
- exception.getMessage(),
- containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING")
- );
- LOG.info("Streams wasn't running. Will try again.");
+ final String message = exception.getMessage();
+ final boolean exceptionNotRunning = message.startsWith("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING");
Review comment:
It might be more robust to use "contains" instead of "startsWith", but I won't insist on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on a change in pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
vvcephei commented on a change in pull request #11129:
URL: https://github.com/apache/kafka/pull/11129#discussion_r678481215
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StoreQueryIntegrationTest.java
##########
@@ -152,11 +150,17 @@ public void shouldQueryOnlyActivePartitionStoresByDefault() throws Exception {
}
return true;
} catch (final InvalidStateStoreException exception) {
- assertThat(
- exception.getMessage(),
- containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING")
- );
- LOG.info("Streams wasn't running. Will try again.");
+ final String message = exception.getMessage();
+ final boolean exceptionNotRunning = message.startsWith("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING");
+ if (exceptionNotRunning) {
+ LOG.info("Streams wasn't running. Will try again.");
+ }
+ final boolean exceptionRebalanced = message.startsWith("The state store, source-table, may have migrated to another instance");
+ if (exceptionRebalanced) {
+ LOG.info("Rebalancing happened. Will try again.");
+ }
+ final boolean expectedException = exceptionNotRunning || exceptionRebalanced;
+ assertThat(expectedException, is(true));
Review comment:
It can sometimes be hard to get the logs for failing tests, so it might be nice to get the failure reason in the actual test output. Check out `Matchers`: there should be a way to compose the checks you're doing here manually. It'll be something like:
```
assertThat(
message,
is(
oneOf(
containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING"),
containsString("The state store, source-table, may have migrated to another instance")
)
)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] showuon commented on a change in pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
showuon commented on a change in pull request #11129:
URL: https://github.com/apache/kafka/pull/11129#discussion_r679004926
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StoreQueryIntegrationTest.java
##########
@@ -153,10 +152,15 @@ public void shouldQueryOnlyActivePartitionStoresByDefault() throws Exception {
return true;
} catch (final InvalidStateStoreException exception) {
assertThat(
- exception.getMessage(),
- containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING")
+ exception.getMessage(),
+ is(
+ oneOf(
+ containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING"),
+ containsString("The state store, source-table, may have migrated to another instance")
+ )
+ )
Review comment:
nit: Could we add a error reason in this assertion, so that when the exception message is not one of these 2 messages, we can know what happened. Ex: "Unexpected exception thrown while getting the value from store."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] patrickstuedi commented on a change in pull request #11129: Fix for flaky test in StoreQueryIntegrationTest
Posted by GitBox <gi...@apache.org>.
patrickstuedi commented on a change in pull request #11129:
URL: https://github.com/apache/kafka/pull/11129#discussion_r679220475
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StoreQueryIntegrationTest.java
##########
@@ -153,10 +152,15 @@ public void shouldQueryOnlyActivePartitionStoresByDefault() throws Exception {
return true;
} catch (final InvalidStateStoreException exception) {
assertThat(
- exception.getMessage(),
- containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING")
+ exception.getMessage(),
+ is(
+ oneOf(
+ containsString("Cannot get state store source-table because the stream thread is PARTITIONS_ASSIGNED, not RUNNING"),
+ containsString("The state store, source-table, may have migrated to another instance")
+ )
+ )
Review comment:
yup, makes sense
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org