cmccabe commented on code in PR #16230:
URL: https://github.com/apache/kafka/pull/16230#discussion_r1672762239
##########
raft/src/main/java/org/apache/kafka/raft/KafkaRaftClient.java:
##########
@@ -3027,6 +3028,11 @@ public long logEndOffset() {
return log.endOffset().offset();
}
+ @Override
+ public KRaftVersion kraftVersion() {
+ return
partitionState.kraftVersionAtOffset(quorum.highWatermark().get().offset());
+ }
Review Comment:
> The offset used should be quorum.highWatermark.map(hwm -> hwm.offset() -
1). The HWM is an exclusive offset
Fixed
> If we want to expose the committed kraft.version then we should change the
signature to Optional<KRaftVersion> kraftVersion().
Hmm. An optional as a return value doesn't help us because we need to
resolve it down to an actual `KRaftVersion`. This ultimately is intended for
use by the metadata cache.
I don't think this is a big issue in practice because the broker shouldn't
be unfenced until the high watermark is known (among other things). So the
cache shouldn't even be accessible. However, having a potential exception here
if highWatermark is unset doesn't feel good. I will just return
`partitionState.lastKraftVersion()` if highWatermark isn't defined.
--
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
