junrao commented on code in PR #15634:
URL: https://github.com/apache/kafka/pull/15634#discussion_r1586840549
##########
core/src/main/scala/kafka/log/UnifiedLog.scala:
##########
@@ -282,15 +282,15 @@ class UnifiedLog(@volatile var logStartOffset: Long,
/**
* Update high watermark with offset metadata. The new high watermark will
be lower
- * bounded by the log start offset and upper bounded by the log end offset.
+ * bounded by the local-log-start-offset and upper bounded by the
log-end-offset.
*
* @param highWatermarkMetadata the suggested high watermark with offset
metadata
* @return the updated high watermark offset
*/
def updateHighWatermark(highWatermarkMetadata: LogOffsetMetadata): Long = {
val endOffsetMetadata = localLog.logEndOffsetMetadata
- val newHighWatermarkMetadata = if (highWatermarkMetadata.messageOffset <
logStartOffset) {
- new LogOffsetMetadata(logStartOffset)
+ val newHighWatermarkMetadata = if (highWatermarkMetadata.messageOffset <
_localLogStartOffset) {
Review Comment:
> I'm not clear on this:
>
> 1. Segments that are eligible for upload to remote storage only when the
lastStableOffset moves beyond the segment-to-be-uploaded-end-offset.
> 2. When all the replicas loses local data (offline partition), then we
consider the data in remote storage also lost. Currently, for this case, we
don't have provision to serve the remote data.
> 3. When firstUnstableOffsetMetadata is empty, we return highWatermark.
With this patch, the highWatermark lower boundary is set to localLogStartOffset
so there won't be an issue.
>
That's true. It's just that that is yet another offset that we need to
bound. I am also not sure if there are other side effects of adjusting HWM and
LSO.
Left some comments on https://github.com/apache/kafka/pull/15825.
--
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
