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 2020/07/15 16:09:51 UTC
[GitHub] [kafka] serjchebotarev opened a new pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
serjchebotarev opened a new pull request #9028:
URL: https://github.com/apache/kafka/pull/9028
In `org.apache.kafka.streams.integration.AbstractResetIntegrationTest`:
- Corrected types of consumer timeout params: `STREAMS_CONSUMER_TIMEOUT ` and `CLEANUP_CONSUMER_TIMEOUT ` from `long` to `int`
- Type-safe conversion from `int` to `String`
### 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456609785
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
...then why extend this abstract class at all? Or rather, why have all these tests in the abstract class and not just in the non-SSL test class?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-660278609
Some follow up nits: The method `add10InputElements` does not throw any exception and we should remove the `throws` statement. Also in `AbstractResetIntegrationTest` in L283 we use `Assert.assertEquals(false, cleanResult)` that should be `Assert.assertFalse(cleanResult)` and L607 we use `Assert.assertEquals(true, cleanResult);` that should be `Assert.assertTrue(cleanResult);`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456791046
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Done. Moved tests to derived class.
Also in the last commit (8218891):
- Refactored how `testId` gets set. Instead of class-level variable it is set via `getTestId()` method which was declared as abstract in `AbstractResetIntegrationTest` (now it cannot be forgotten to be set in derived classes).
- Removed `appID` instance variable (removed instance-level state).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-669823727
@vvcephei & @mjsax this one seems to get stalled a bit - could you let me know if anything else is to be corrected here or should we move on with retesting the changes?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-673036155
@serjchebotarev he's out of office this week, just fyi.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-659701794
Ok to test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vvcephei commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
vvcephei commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-659701606
Test this please
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456611805
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Not exactly sure. Guess it was just a random decision to do it this way.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r457756031
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/ResetIntegrationTest.java
##########
@@ -68,52 +92,224 @@ public void after() throws Exception {
}
@Test
- public void testReprocessingFromScratchAfterResetWithoutIntermediateUserTopic() throws Exception {
- super.testReprocessingFromScratchAfterResetWithoutIntermediateUserTopic();
+ public void shouldNotAllowToResetWhileStreamsIsRunning() {
+ final String appID = getTestId() + "-not-reset-during-runtime";
Review comment:
A new safeUniqueTestName method was added to IntegrationTestUtils recently, can we port this class over to using that instead of hard-coding a semi-descriptive suffix to the appId in each test?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456067480
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Tried on this, partially it went good, but several tests fail under `ResetIntegrationWithSslTest`, namely:
- `shouldNotAllowToResetWhileStreamsIsRunning`
- `shouldNotAllowToResetWhenInputTopicAbsent`
- `shouldNotAllowToResetWhenIntermediateTopicAbsent`
From output of `shouldNotAllowToResetWhileStreamsIsRunning` there are some log lines that mention SSL handshake failure:
```
[2020-07-16 23:00:20,764] INFO stream-thread [reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d-StreamThread-1] State transition from PARTITIONS_ASSIGNED to RUNNING (org.apache.kafka.streams.processor.internals.StreamThread:220)
[2020-07-16 23:00:20,766] INFO stream-client [reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d] State transition from REBALANCING to RUNNING (org.apache.kafka.streams.KafkaStreams:283)
[2020-07-16 23:00:20,771] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:21,067] INFO [Consumer clientId=reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d-StreamThread-1-consumer, groupId=reset-with-ssl-integration-test-not-reset-during-runtime] Found no committed offset for partition inputTopic-0 (org.apache.kafka.clients.consumer.internals.ConsumerCoordinator:1349)
[2020-07-16 23:00:21,093] INFO [Consumer clientId=reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d-StreamThread-1-consumer, groupId=reset-with-ssl-integration-test-not-reset-during-runtime] Resetting offset for partition inputTopic-0 to offset 0. (org.apache.kafka.clients.consumer.internals.SubscriptionState:397)
[2020-07-16 23:00:21,174] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:21,548] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:21,798] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,028] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,329] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,556] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,793] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
```
Was not able to dig deeper into what could be the reason for this. For now just left these three tests in the base class without `@Test` annotation and calling them as before in `ResetIntegrationTest` only.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-673049249
@ableegoldman alright, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456067480
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Tried on this, partially it went good, but several tests fail under `ResetIntegrationWithSslTest`, namely:
- `shouldNotAllowToResetWhileStreamsIsRunning`
- `shouldNotAllowToResetWhenInputTopicAbsent`
- `shouldNotAllowToResetWhenIntermediateTopicAbsent`
From output of `shouldNotAllowToResetWhileStreamsIsRunning` there are some log lines that mention SSL handshake failure:
```
...
[2020-07-16 23:00:20,764] INFO stream-thread [reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d-StreamThread-1] State transition from PARTITIONS_ASSIGNED to RUNNING (org.apache.kafka.streams.processor.internals.StreamThread:220)
[2020-07-16 23:00:20,766] INFO stream-client [reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d] State transition from REBALANCING to RUNNING (org.apache.kafka.streams.KafkaStreams:283)
[2020-07-16 23:00:20,771] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:21,067] INFO [Consumer clientId=reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d-StreamThread-1-consumer, groupId=reset-with-ssl-integration-test-not-reset-during-runtime] Found no committed offset for partition inputTopic-0 (org.apache.kafka.clients.consumer.internals.ConsumerCoordinator:1349)
[2020-07-16 23:00:21,093] INFO [Consumer clientId=reset-with-ssl-integration-test-not-reset-during-runtime-6971acc7-e471-4973-a652-5a09b7dce10d-StreamThread-1-consumer, groupId=reset-with-ssl-integration-test-not-reset-during-runtime] Resetting offset for partition inputTopic-0 to offset 0. (org.apache.kafka.clients.consumer.internals.SubscriptionState:397)
[2020-07-16 23:00:21,174] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:21,548] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:21,798] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,028] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,329] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,556] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
[2020-07-16 23:00:22,793] INFO [SocketServer brokerId=0] Failed authentication with /127.0.0.1 (SSL handshake failed) (org.apache.kafka.common.network.Selector:616)
org.apache.kafka.streams.integration.ResetIntegrationWithSslTest > shouldNotAllowToResetWhileStreamsIsRunning SKIPPED
> Task :streams:test FAILED
:streams:test (Thread[Execution worker for ':',5,main]) completed. Took 14.586 secs.
FAILURE: Build failed with an exception.
```
Was not able to dig deeper into what could be the reason for this. For now just left these three tests in the base class without `@Test` annotation and calling them as before in `ResetIntegrationTest` only.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r458064223
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/ResetIntegrationTest.java
##########
@@ -68,52 +92,224 @@ public void after() throws Exception {
}
@Test
- public void testReprocessingFromScratchAfterResetWithoutIntermediateUserTopic() throws Exception {
- super.testReprocessingFromScratchAfterResetWithoutIntermediateUserTopic();
+ public void shouldNotAllowToResetWhileStreamsIsRunning() {
+ final String appID = getTestId() + "-not-reset-during-runtime";
Review comment:
Done! :smile:
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-660484592
@ableegoldman & @mjsax please have a look at updates.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-659095839
Retest this please.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-658858961
@chia7712, @mjsax, @ableegoldman could you please review this fix?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-662341113
Rebased to resolve merge conflict.
@mjsax could you please give it a second look?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456068727
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -151,7 +151,7 @@ private void prepareConfigs() {
streamsConfig.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, 100);
streamsConfig.put(ConsumerConfig.HEARTBEAT_INTERVAL_MS_CONFIG, 100);
streamsConfig.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
- streamsConfig.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, "" + STREAMS_CONSUMER_TIMEOUT);
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r455916364
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -151,7 +151,7 @@ private void prepareConfigs() {
streamsConfig.put(StreamsConfig.COMMIT_INTERVAL_MS_CONFIG, 100);
streamsConfig.put(ConsumerConfig.HEARTBEAT_INTERVAL_MS_CONFIG, 100);
streamsConfig.put(ConsumerConfig.AUTO_OFFSET_RESET_CONFIG, "earliest");
- streamsConfig.put(ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG, "" + STREAMS_CONSUMER_TIMEOUT);
Review comment:
Hah, this was pretty janky. Good catch
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Why are none of these tests...actually tests? Can you also fix this, ie add `@Test` annotations to all the tests here?
I think you can then simplify the two tests that extend this abstract test class (`ResetIntegrationTest` and `ResetIntegrationWithSslTest`) and just remove all the tests that just call `super.testXXX` -- they should automatically run all of the tests in this class
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456101096
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Thanks for looking into this! It definitely doesn't sound good that some of these tests don't pass with SSL. Maybe there's just some additional setup that's needed for these tests? 🤔
Anyways, we don't need to solve all that in this PR. We can revisit the issue once this is merged
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ableegoldman commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
ableegoldman commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456615189
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Okay, maybe I gave the wrong advise here and instead of consolidating the two classes we should have separated them.
WDYT about just moving all but the three tests that we want to run with SSL out of `AbstractResetIntegrationTest` and into `ResetIntegrationTest` instead?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax merged pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax merged pull request #9028:
URL: https://github.com/apache/kafka/pull/9028
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-678457711
Thanks for the patch @serjchebotarev! Merged to `trunk`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456616494
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Works for me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-660484491
In 24456ae addressed comments from @mjsax
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on a change in pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on a change in pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#discussion_r456608124
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/AbstractResetIntegrationTest.java
##########
@@ -265,7 +265,7 @@ public void shouldNotAllowToResetWhenIntermediateTopicAbsent() throws Exception
public void testResetWhenLongSessionTimeoutConfiguredWithForceOption() throws Exception {
Review comment:
Well, actually, we don't want to run all test 2x. The Ssl test only executes 3 test (on purpose) as the other tests would be repetitive and don't add value. -- Given that we alway try to keep test execution time short, we can save about 1 minute if we avoid running the tests twice? Thoughts?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mjsax commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
mjsax commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-670689958
Retest this please.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] serjchebotarev commented on pull request #9028: KAFKA-10035: Safer conversion of consumer timeout parameters
Posted by GitBox <gi...@apache.org>.
serjchebotarev commented on pull request #9028:
URL: https://github.com/apache/kafka/pull/9028#issuecomment-672683714
Looks like no issues related to the changes in this PR. @mjsax could you please suggest what would be the next thing to do on this one?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org