hachikuji commented on a change in pull request #9590:
URL: https://github.com/apache/kafka/pull/9590#discussion_r610167018
##########
File path: core/src/main/scala/kafka/log/Log.scala
##########
@@ -760,6 +768,12 @@ class Log(@volatile private var _dir: File,
// must fall within the range of existing segment(s). If we cannot find
such a segment, it means the deletion
// of that segment was successful. In such an event, we should simply
rename the .swap to .log without having to
// do a replace with an existing segment.
+ //
+ // For case 1 (log cleaning), we may have old segments before or after
the swap segment that were cleaned.
Review comment:
Initially I interpreted the comment as suggesting a stronger statement
about how each swapped segment related to the set of segments that it would
replace. Perhaps it was really the comment that the "swap segments be composed
of the original set" which confused me.
I would suggest that we use more precise language. For example, can we say
that all offsets in the swapped segment must be present in the segments that
are to be replaced? It would also be helpful to clarify the following sentence:
> If we cannot find such a segment, it means the deletion of that segment
was successful.
Really we are referring to a group of segments. How does the conclusion that
deletion was successful follow from the fact that the swapped segment is
"composed" of the original segments?
--
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
