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/04/26 06:42:21 UTC

[GitHub] [kafka] dengziming opened a new pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

dengziming opened a new pull request #10592:
URL: https://github.com/apache/kafka/pull/10592


   *More detailed description of your change*
   ![image](https://user-images.githubusercontent.com/26023240/115959686-01b41080-a540-11eb-88b6-a7c668ddfa38.png)
   
   1. `LogSegmentsTest` will create some redundant files, we should remove them after the test.
   2. We also need to close LogSegment after the test.
   
   *Summary of testing strategy (including rationale)*
   Test locally, will not generate any redundant files.
   
   ### 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] kowshik commented on a change in pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#discussion_r619682506



##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -35,6 +39,17 @@ class LogSegmentsTest {
     LogTestUtils.createSegment(offset, logDir, indexIntervalBytes, time)
   }
 
+  @BeforeEach
+  def setup(): Unit = {
+    logDir = TestUtils.tempDir()
+  }
+
+  @AfterEach
+  def teardown(): Unit = {
+    segmentsBuffer.foreach(_.close())

Review comment:
       The segments are already closed from inside `LogSegments#close()` and therefore can be skipped in the `tearDown()`. https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/log/LogSegments.scala#L78

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -129,12 +149,13 @@ class LogSegmentsTest {
     assertEquals(Seq(seg3), segments.values(3, 4).toSeq)
     assertEquals(Seq(), segments.values(4, 4).toSeq)
     assertEquals(Seq(seg4), segments.values(4, 5).toSeq)
-

Review comment:
       Lets add a `segments.close()` call here.

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -129,12 +149,13 @@ class LogSegmentsTest {
     assertEquals(Seq(seg3), segments.values(3, 4).toSeq)
     assertEquals(Seq(), segments.values(4, 4).toSeq)
     assertEquals(Seq(seg4), segments.values(4, 5).toSeq)
-
   }
 
   @Test
   def testClosestMatchOperations(): Unit = {
     val segments = new LogSegments(topicPartition)

Review comment:
       Lets add a `segments.close()` call at the end of this function.

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -18,15 +18,19 @@ package kafka.log
 
 import java.io.File
 
+import kafka.utils.TestUtils
 import org.apache.kafka.common.TopicPartition
-import org.apache.kafka.common.utils.Time
+import org.apache.kafka.common.utils.{Time, Utils}
 import org.junit.jupiter.api.Assertions._
-import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
+
+import scala.collection.mutable
 
 class LogSegmentsTest {
 
   val topicPartition = new TopicPartition("topic", 0)
   var logDir: File = _
+  val segmentsBuffer: mutable.ArrayBuffer[LogSegments] = mutable.ArrayBuffer[LogSegments]()

Review comment:
       This buffer can be removed, as I described below by calling `LogSegments#close()` inside each test.

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -35,6 +39,17 @@ class LogSegmentsTest {
     LogTestUtils.createSegment(offset, logDir, indexIntervalBytes, time)
   }
 
+  @BeforeEach
+  def setup(): Unit = {
+    logDir = TestUtils.tempDir()
+  }
+
+  @AfterEach
+  def teardown(): Unit = {
+    segmentsBuffer.foreach(_.close())

Review comment:
       The segments are already closed from inside `LogSegments#close()` and therefore closing these can be skipped in the `tearDown()`. https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/log/LogSegments.scala#L78




-- 
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] dengziming commented on pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#issuecomment-826196454


   @kowshik , Thank you for your suggestions, I have rewritten the code according to your comments, PTAL again.


-- 
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] kowshik commented on a change in pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#discussion_r619682506



##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -35,6 +39,17 @@ class LogSegmentsTest {
     LogTestUtils.createSegment(offset, logDir, indexIntervalBytes, time)
   }
 
+  @BeforeEach
+  def setup(): Unit = {
+    logDir = TestUtils.tempDir()
+  }
+
+  @AfterEach
+  def teardown(): Unit = {
+    segmentsBuffer.foreach(_.close())

Review comment:
       The segments are already closed from inside `LogSegments#close()` and therefore closing these can be skipped in the `tearDown()`. https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/log/LogSegments.scala#L78




-- 
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] chia7712 commented on pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#issuecomment-827477372


   @dengziming nice find and thanks for your patch. Those files are noisy to 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] dengziming commented on pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#issuecomment-826089682






-- 
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] dengziming commented on pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#issuecomment-826089682


   Hello, @kowshik PTAL, also cc @junrao .


-- 
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] kowshik commented on a change in pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
kowshik commented on a change in pull request #10592:
URL: https://github.com/apache/kafka/pull/10592#discussion_r619682506



##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -35,6 +39,17 @@ class LogSegmentsTest {
     LogTestUtils.createSegment(offset, logDir, indexIntervalBytes, time)
   }
 
+  @BeforeEach
+  def setup(): Unit = {
+    logDir = TestUtils.tempDir()
+  }
+
+  @AfterEach
+  def teardown(): Unit = {
+    segmentsBuffer.foreach(_.close())

Review comment:
       The segments are already closed from inside `LogSegments#close()` and therefore can be skipped in the `tearDown()`. https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/log/LogSegments.scala#L78

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -129,12 +149,13 @@ class LogSegmentsTest {
     assertEquals(Seq(seg3), segments.values(3, 4).toSeq)
     assertEquals(Seq(), segments.values(4, 4).toSeq)
     assertEquals(Seq(seg4), segments.values(4, 5).toSeq)
-

Review comment:
       Lets add a `segments.close()` call here.

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -129,12 +149,13 @@ class LogSegmentsTest {
     assertEquals(Seq(seg3), segments.values(3, 4).toSeq)
     assertEquals(Seq(), segments.values(4, 4).toSeq)
     assertEquals(Seq(seg4), segments.values(4, 5).toSeq)
-
   }
 
   @Test
   def testClosestMatchOperations(): Unit = {
     val segments = new LogSegments(topicPartition)

Review comment:
       Lets add a `segments.close()` call at the end of this function.

##########
File path: core/src/test/scala/unit/kafka/log/LogSegmentsTest.scala
##########
@@ -18,15 +18,19 @@ package kafka.log
 
 import java.io.File
 
+import kafka.utils.TestUtils
 import org.apache.kafka.common.TopicPartition
-import org.apache.kafka.common.utils.Time
+import org.apache.kafka.common.utils.{Time, Utils}
 import org.junit.jupiter.api.Assertions._
-import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
+
+import scala.collection.mutable
 
 class LogSegmentsTest {
 
   val topicPartition = new TopicPartition("topic", 0)
   var logDir: File = _
+  val segmentsBuffer: mutable.ArrayBuffer[LogSegments] = mutable.ArrayBuffer[LogSegments]()

Review comment:
       This buffer can be removed, as I described below by calling `LogSegments#close()` inside 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] junrao merged pull request #10592: MINOR: Remove redudant test files and close LogSegment after test

Posted by GitBox <gi...@apache.org>.
junrao merged pull request #10592:
URL: https://github.com/apache/kafka/pull/10592


   


-- 
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