[GitHub] [kafka] mjsax commented on a change in pull request #10668: MINOR: Apply try-with-resource to KafkaStreamsTest

2021-06-02 Thread GitBox 


mjsax commented on a change in pull request #10668:
URL: https://github.com/apache/kafka/pull/10668#discussion_r644184061



##
File path: streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
##
@@ -491,25 +493,23 @@ public void testStateThreadClose() throws Exception {
 () -> streams.localThreadsMetadata().stream().allMatch(t -> 
t.threadState().equals("DEAD")),
 "Streams never stopped"
 );
-} finally {
 streams.close();

Review comment:
   I missed the fact that we moved the `waitForCondition` check _inside_ of 
the try-catch block... For this case, we need to call `close` explicitly of 
course, as we are still in the block and `close()` is not auto-called yet...
   
   Sorry for the confusion.




-- 
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:
us...@infra.apache.org

[GitHub] [kafka] mjsax commented on a change in pull request #10668: MINOR: Apply try-with-resource to KafkaStreamsTest

2021-05-26 Thread GitBox 


mjsax commented on a change in pull request #10668:
URL: https://github.com/apache/kafka/pull/10668#discussion_r640127195



##
File path: streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
##
@@ -525,14 +525,13 @@ public void testStateGlobalThreadClose() throws Exception 
{
 () -> streams.state() == KafkaStreams.State.PENDING_ERROR,
 "Thread never stopped."
 );
-} finally {
 streams.close();

Review comment:
   Sams comment as above. The Java compiler will insert a `close()` call 
and there is no need to explicitly call it any longer.




-- 
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:
us...@infra.apache.org

[GitHub] [kafka] mjsax commented on a change in pull request #10668: MINOR: Apply try-with-resource to KafkaStreamsTest

2021-05-26 Thread GitBox 


mjsax commented on a change in pull request #10668:
URL: https://github.com/apache/kafka/pull/10668#discussion_r640126531



##
File path: streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
##
@@ -491,25 +493,23 @@ public void testStateThreadClose() throws Exception {
 () -> streams.localThreadsMetadata().stream().allMatch(t -> 
t.threadState().equals("DEAD")),
 "Streams never stopped"
 );
-} finally {
 streams.close();

Review comment:
   As `KafkaStreams` implements the `AutoCloseable` interface now, 
`close()` should be called automatically when the `try {}` block is left -- 
that is the whole purpose of `AutoClosable` and try-with-resource construct -- 
it frees you up to call `close()` explicitly (so you cannot forget any longer).




-- 
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:
us...@infra.apache.org

[GitHub] [kafka] mjsax commented on a change in pull request #10668: MINOR: Apply try-with-resource to KafkaStreamsTest

2021-05-11 Thread GitBox 


mjsax commented on a change in pull request #10668:
URL: https://github.com/apache/kafka/pull/10668#discussion_r630602355



##
File path: streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
##
@@ -553,141 +552,152 @@ public void 
testInitializesAndDestroysMetricsReporters() {
 
 @Test
 public void testCloseIsIdempotent() {
-final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time);
-streams.close();
-final int closeCount = MockMetricsReporter.CLOSE_COUNT.get();
+try (final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time)) {
+streams.close();
+final int closeCount = MockMetricsReporter.CLOSE_COUNT.get();
 
-streams.close();
-Assert.assertEquals("subsequent close() calls should do nothing",
-closeCount, MockMetricsReporter.CLOSE_COUNT.get());
+streams.close();
+Assert.assertEquals("subsequent close() calls should do nothing",
+closeCount, MockMetricsReporter.CLOSE_COUNT.get());
+}
 }
 
 @Test
 public void shouldAddThreadWhenRunning() throws InterruptedException {
 props.put(StreamsConfig.NUM_STREAM_THREADS_CONFIG, 1);
-final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time);
-streams.start();
-final int oldSize = streams.threads.size();
-TestUtils.waitForCondition(() -> streams.state() == 
KafkaStreams.State.RUNNING, 15L, "wait until running");
-assertThat(streams.addStreamThread(), 
equalTo(Optional.of("processId-StreamThread-" + 2)));
-assertThat(streams.threads.size(), equalTo(oldSize + 1));
+try (final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time)) {
+streams.start();
+final int oldSize = streams.threads.size();
+TestUtils.waitForCondition(() -> streams.state() == 
KafkaStreams.State.RUNNING, 15L, "wait until running");
+assertThat(streams.addStreamThread(), 
equalTo(Optional.of("processId-StreamThread-" + 2)));
+assertThat(streams.threads.size(), equalTo(oldSize + 1));
+}
 }
 
 @Test
 public void shouldNotAddThreadWhenCreated() {
-final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time);
-final int oldSize = streams.threads.size();
-assertThat(streams.addStreamThread(), equalTo(Optional.empty()));
-assertThat(streams.threads.size(), equalTo(oldSize));
+try (final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time)) {
+final int oldSize = streams.threads.size();
+assertThat(streams.addStreamThread(), equalTo(Optional.empty()));
+assertThat(streams.threads.size(), equalTo(oldSize));
+}
 }
 
 @Test
 public void shouldNotAddThreadWhenClosed() {
-final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time);
-final int oldSize = streams.threads.size();
-streams.close();
-assertThat(streams.addStreamThread(), equalTo(Optional.empty()));
-assertThat(streams.threads.size(), equalTo(oldSize));
+try (final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time)) {
+final int oldSize = streams.threads.size();
+streams.close();
+assertThat(streams.addStreamThread(), equalTo(Optional.empty()));
+assertThat(streams.threads.size(), equalTo(oldSize));
+}
 }
 
 @Test
 public void shouldNotAddThreadWhenError() {
-final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time);
-final int oldSize = streams.threads.size();
-streams.start();
-globalStreamThread.shutdown();
-assertThat(streams.addStreamThread(), equalTo(Optional.empty()));
-assertThat(streams.threads.size(), equalTo(oldSize));
+try (final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time)) {
+final int oldSize = streams.threads.size();
+streams.start();
+globalStreamThread.shutdown();
+assertThat(streams.addStreamThread(), equalTo(Optional.empty()));
+assertThat(streams.threads.size(), equalTo(oldSize));
+}
 }
 
 @Test
 public void shouldNotReturnDeadThreads() {
-final KafkaStreams streams = new 
KafkaStreams(getBuilderWithSource().build(), props, supplier, time);
-streams.start();
-streamThreadOne.shutdown();
-final Set threads = streams.localThreadsMetadata();
+final Set threads;
+try (final KafkaStreams streams

[GitHub] [kafka] mjsax commented on a change in pull request #10668: MINOR: Apply try-with-resource to KafkaStreamsTest

2021-05-11 Thread GitBox 


mjsax commented on a change in pull request #10668:
URL: https://github.com/apache/kafka/pull/10668#discussion_r630601702



##
File path: streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
##
@@ -525,14 +525,13 @@ public void testStateGlobalThreadClose() throws Exception 
{
 () -> streams.state() == KafkaStreams.State.PENDING_ERROR,
 "Thread never stopped."
 );
-} finally {
 streams.close();

Review comment:
   As above. (Maybe similar elsewhere; won't comment on it explicitly below)




-- 
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:
us...@infra.apache.org

[GitHub] [kafka] mjsax commented on a change in pull request #10668: MINOR: Apply try-with-resource to KafkaStreamsTest

2021-05-11 Thread GitBox 


mjsax commented on a change in pull request #10668:
URL: https://github.com/apache/kafka/pull/10668#discussion_r630601302



##
File path: streams/src/test/java/org/apache/kafka/streams/KafkaStreamsTest.java
##
@@ -491,25 +493,23 @@ public void testStateThreadClose() throws Exception {
 () -> streams.localThreadsMetadata().stream().allMatch(t -> 
t.threadState().equals("DEAD")),
 "Streams never stopped"
 );
-} finally {
 streams.close();

Review comment:
   We can remove this line now. `close()` will be cause automatically using 
try-with-resource clause.




-- 
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:
us...@infra.apache.org