[GitHub] [kafka] abbccdda commented on a change in pull request #9332: KAFKA-10511; Ensure monotonic start epoch/offset updates in `MockLog`

[GitBox <gi...@apache.org>]

abbccdda commented on a change in pull request #9332:
URL: https://github.com/apache/kafka/pull/9332#discussion_r494062676



##########
File path: raft/src/test/java/org/apache/kafka/raft/MockLogTest.java
##########
@@ -370,6 +365,23 @@ public void testReadOutOfRangeOffset() {
             Isolation.UNCOMMITTED));
     }
 
+    @Test
+    public void testMonotonicEpochStartOffset() {
+        appendBatch(5, 1);
+        assertEquals(5L, log.endOffset().offset);
+
+        log.initializeLeaderEpoch(2);
+        assertEquals(Optional.of(new OffsetAndEpoch(5L, 1)), log.endOffsetForEpoch(1));
+        assertEquals(Optional.of(new OffsetAndEpoch(5L, 2)), log.endOffsetForEpoch(2));
+
+        // Initialize a new epoch at the same end offset. The epoch cache ensures
+        // that the start offset of each retained epoch increases monotonically.

Review comment:
       I thought this is a test for end offset?

##########
File path: raft/src/test/java/org/apache/kafka/raft/RaftEventSimulationTest.java
##########
@@ -712,11 +712,6 @@ void initializeElection(ElectionState election) {
 
             nodes.values().forEach(state -> {
                 state.store.writeElectionState(election);
-                if (election.hasLeader()) {

Review comment:
       So the reason that this is safe is because we no longer try assigning offset other than the end offset?

##########
File path: raft/src/test/java/org/apache/kafka/raft/MockLog.java
##########
@@ -310,10 +310,10 @@ public LogFetchInfo read(long startOffset, Isolation isolation) {
     }
 
     @Override
-    public void assignEpochStartOffset(int epoch, long startOffset) {
-        if (startOffset != endOffset().offset)
-            throw new IllegalArgumentException(
-                "Can only assign epoch for the end offset " + endOffset().offset + ", but get offset " + startOffset);
+    public void initializeLeaderEpoch(int epoch) {
+        long startOffset = endOffset().offset;
+        epochStartOffsets.removeIf(epochStartOffset ->

Review comment:
       So the idea is to wipe out the older epoch (as epoch - 1)'s end offset, and the search for (epoch - 1) would give (epoch - 2) end offset? In the case where we put leader change message in mock log, this should never happen right?




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