satishd commented on code in PR #14034:
URL: https://github.com/apache/kafka/pull/14034#discussion_r1451284145


##########
core/src/main/scala/kafka/log/LogLoader.scala:
##########
@@ -489,16 +488,16 @@ class LogLoader(
    *
    * @param segmentsToDelete The log segments to schedule for deletion
    */
-  private def removeAndDeleteSegmentsAsync(segmentsToDelete: 
Iterable[LogSegment]): Unit = {
-    if (segmentsToDelete.nonEmpty) {
+  private def removeAndDeleteSegmentsAsync(segmentsToDelete: 
java.util.Collection[LogSegment]): Unit = {
+    if (!segmentsToDelete.isEmpty) {
       // Most callers hold an iterator into the `params.segments` collection 
and
       // `removeAndDeleteSegmentAsync` mutates it by removing the deleted 
segment. Therefore,
       // we should force materialization of the iterator here, so that results 
of the iteration
       // remain valid and deterministic. We should also pass only the 
materialized view of the
       // iterator to the logic that deletes the segments.
-      val toDelete = segmentsToDelete.toList
-      info(s"Deleting segments as part of log recovery: 
${toDelete.mkString(",")}")
-      toDelete.foreach { segment =>
+      val toDelete = new util.ArrayList[LogSegment](segmentsToDelete)
+      info(s"Deleting segments as part of log recovery: 
${LocalLog.mkString(toDelete.iterator(), ",")}")

Review Comment:
   Thanks for letting me know about `Utils.join`, I was not aware of it.



##########
core/src/test/scala/unit/kafka/log/LocalLogTest.scala:
##########
@@ -362,8 +363,8 @@ class LocalLogTest {
     }
     assertEquals(5, log.segments.numberOfSegments)
     assertNotEquals(10L, log.segments.activeSegment.baseOffset)
-    val expected = log.segments.values.asScala.toVector
-    val deleted = log.truncateFullyAndStartAt(10L)
+    val expected = new util.ArrayList(log.segments.values)
+    val deleted = new util.ArrayList(log.truncateFullyAndStartAt(10L))

Review Comment:
   They give different types like java `ConcurrentSkipListMap.Values` and 
`ArrayList`, and the equality check will fail. Wrapped around the same 
ArrayList instance for their equality checks. 
   Followed similar checks in LocalLogTest and UnifiedLogTest.



-- 
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-unsubscr...@kafka.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to