You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "majialoong (via GitHub)" <gi...@apache.org> on 2025/07/20 17:45:44 UTC
[PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
majialoong opened a new pull request, #20205:
URL: https://github.com/apache/kafka/pull/20205
Optimize `RemoteLogManager#buildFilteredLeaderEpochMap` .
Use JMH test :
```
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class LeaderEpochMapBenchmark {
@Param({"1000", "10000", "100000"})
private int mapSize;
NavigableMap<Integer, Long> testMap;
@Setup
public void setup() {
testMap = new TreeMap<>();
for (int i = 0; i < mapSize; i++) {
/**
* 0,0
* 1,0
* 2,1
* 3,1
* ...
*/
testMap.put(i, (long) (i / 2));
}
}
@Benchmark
public NavigableMap<Integer, Long> testOriginal() {
// before optimize (make method public)
return RemoteLogManager.buildFilteredLeaderEpochMap(testMap);
}
@Benchmark
public NavigableMap<Integer, Long> testOptimized() {
// after optimize (make method public)
return RemoteLogManager.buildFilteredLeaderEpochMap2(testMap);
}
}
```
Command :
``` ./jmh-benchmarks/jmh.sh LeaderEpochMapBenchmark ```
Result :
<img width="1528" height="370" alt="image" src="https://github.com/user-attachments/assets/9f912a40-2c88-47fc-8eea-e8f063d7c320" />
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "kamalcph (via GitHub)" <gi...@apache.org>.
kamalcph commented on code in PR #20205:
URL: https://github.com/apache/kafka/pull/20205#discussion_r2225075612
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
@@ -1639,20 +1639,20 @@ static boolean isRemoteSegmentWithinLeaderEpochs(RemoteLogSegmentMetadata segmen
*/
// Visible for testing
static NavigableMap<Integer, Long> buildFilteredLeaderEpochMap(NavigableMap<Integer, Long> leaderEpochs) {
Review Comment:
Could you also remove the example mentioned in the javadoc? The scenario is already covered as part of unit test.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on code in PR #20205:
URL: https://github.com/apache/kafka/pull/20205#discussion_r2225729707
##########
storage/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerTest.java:
##########
@@ -1847,6 +1847,18 @@ public void testBuildFilteredLeaderEpochMap() {
NavigableMap<Integer, Long> refinedLeaderEpochMap = RemoteLogManager.buildFilteredLeaderEpochMap(leaderEpochToStartOffset);
assertEquals(expectedLeaderEpochs, refinedLeaderEpochMap);
+
+
+ TreeMap<Integer, Long> leaderEpochToStartOffset2 = new TreeMap<>();
Review Comment:
Thanks, I will add the unit test results below to verify the consistency of the methods before and after this modification.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on PR #20205:
URL: https://github.com/apache/kafka/pull/20205#issuecomment-3104130169
In the original method, there is no need to count the epochs without messages（epochsWithNoMessages） first and then remove them. We can directly calculate and get the results.
Use JMH test :
```
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@State(Scope.Thread)
public class LeaderEpochMapBenchmark {
@Param({"1000", "10000", "100000"})
private int mapSize;
NavigableMap<Integer, Long> testMap;
@Setup
public void setup() {
testMap = new TreeMap<>();
for (int i = 0; i < mapSize; i++) {
/**
* 0,0
* 1,0
* 2,1
* 3,1
* ...
*/
testMap.put(i, (long) (i / 2));
}
}
@Benchmark
public NavigableMap<Integer, Long> testOriginal() {
// before optimize (make method public)
return RemoteLogManager.buildFilteredLeaderEpochMap(testMap);
}
@Benchmark
public NavigableMap<Integer, Long> testOptimized() {
// after optimize (make method public)
return RemoteLogManager.buildFilteredLeaderEpochMap2(testMap);
}
}
```
Command :
``` ./jmh-benchmarks/jmh.sh LeaderEpochMapBenchmark ```
Result :
<img width="1528" height="370" alt="image" src="https://github.com/user-attachments/assets/9f912a40-2c88-47fc-8eea-e8f063d7c320" />
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on PR #20205:
URL: https://github.com/apache/kafka/pull/20205#issuecomment-3109487326
Add unit tests `testBuildFilteredLeaderEpochMapModify` in `RemoteLogManagerTest` to verify the output consistency of the method before and after optimization.
Randomly generate leaderEpochs and iterate 100000 times for verification.
```
@Test
public void testBuildFilteredLeaderEpochMapModify() {
int testIterations = 100000;
for (int i = 0; i < testIterations; i++) {
TreeMap<Integer, Long> leaderEpochToStartOffset = generateRandomLeaderEpochAndStartOffset();
// before optimize
NavigableMap<Integer, Long> optimizeBefore = RemoteLogManager.buildFilteredLeaderEpochMap(leaderEpochToStartOffset);
// after optimize
NavigableMap<Integer, Long> optimizeAfter = RemoteLogManager.buildFilteredLeaderEpochMap2(leaderEpochToStartOffset);
assertEquals(optimizeBefore, optimizeAfter);
}
}
private static TreeMap<Integer, Long> generateRandomLeaderEpochAndStartOffset() {
TreeMap<Integer, Long> map = new TreeMap<>();
Random random = new Random();
int numEntries = random.nextInt(100000);
long lastStartOffset = 0;
for (int i = 0; i < numEntries; i++) {
// generate a leader epoch
int leaderEpoch = random.nextInt(100000);
long startOffset;
// generate a random start offset , or use the last start offset
if (i > 0 && random.nextDouble() < 0.2) {
startOffset = lastStartOffset;
} else {
startOffset = Math.abs(random.nextLong()) % 100000;
}
lastStartOffset = startOffset;
map.put(leaderEpoch, startOffset);
}
return map;
}
```
Command:
``` ./gradlew storage:test --tests RemoteLogManagerTest```
Result:
All unit tests passed.
<img width="1258" height="424" alt="image" src="https://github.com/user-attachments/assets/7d9fc3b5-3bbc-440f-b1cf-3a2a5f97557a" />
<img width="411" height="66" alt="image" src="https://github.com/user-attachments/assets/22a0b443-88e8-43d2-a3f2-51266935ed34" />
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on PR #20205:
URL: https://github.com/apache/kafka/pull/20205#issuecomment-3109586464
Hello @chia7712 , @kamalcph . I made some additions and adjustments. PTAL, thanks !
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on PR #20205:
URL: https://github.com/apache/kafka/pull/20205#issuecomment-3097217577
@chia7712 @kamalcph @frankvicky If have chance, please take a look this RP, thanks !
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on code in PR #20205:
URL: https://github.com/apache/kafka/pull/20205#discussion_r2225737898
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
@@ -1639,20 +1639,20 @@ static boolean isRemoteSegmentWithinLeaderEpochs(RemoteLogSegmentMetadata segmen
*/
// Visible for testing
static NavigableMap<Integer, Long> buildFilteredLeaderEpochMap(NavigableMap<Integer, Long> leaderEpochs) {
- List<Integer> epochsWithNoMessages = new ArrayList<>();
+ if (leaderEpochs.size() <= 1) return leaderEpochs;
Review Comment:
You're right, this isn't necessary.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20205:
URL: https://github.com/apache/kafka/pull/20205#discussion_r2223634384
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
@@ -1639,20 +1639,20 @@ static boolean isRemoteSegmentWithinLeaderEpochs(RemoteLogSegmentMetadata segmen
*/
// Visible for testing
static NavigableMap<Integer, Long> buildFilteredLeaderEpochMap(NavigableMap<Integer, Long> leaderEpochs) {
- List<Integer> epochsWithNoMessages = new ArrayList<>();
+ if (leaderEpochs.size() <= 1) return leaderEpochs;
Review Comment:
Is this early return necessary?
##########
storage/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteLogManagerTest.java:
##########
@@ -1847,6 +1847,18 @@ public void testBuildFilteredLeaderEpochMap() {
NavigableMap<Integer, Long> refinedLeaderEpochMap = RemoteLogManager.buildFilteredLeaderEpochMap(leaderEpochToStartOffset);
assertEquals(expectedLeaderEpochs, refinedLeaderEpochMap);
+
+
+ TreeMap<Integer, Long> leaderEpochToStartOffset2 = new TreeMap<>();
Review Comment:
It would be useful to have a unit test to loop over 1000 times, using random input to ensure before/after methods have same output.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "majialoong (via GitHub)" <gi...@apache.org>.
majialoong commented on code in PR #20205:
URL: https://github.com/apache/kafka/pull/20205#discussion_r2225715309
##########
storage/src/main/java/org/apache/kafka/server/log/remote/storage/RemoteLogManager.java:
##########
@@ -1639,20 +1639,20 @@ static boolean isRemoteSegmentWithinLeaderEpochs(RemoteLogSegmentMetadata segmen
*/
// Visible for testing
static NavigableMap<Integer, Long> buildFilteredLeaderEpochMap(NavigableMap<Integer, Long> leaderEpochs) {
Review Comment:
Thanks for your reminder, i will remove it.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Optimize RemoteLogManager#buildFilteredLeaderEpochMap [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20205:
URL: https://github.com/apache/kafka/pull/20205
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org