Skip to content

Commit 1db1ae7

Browse files
authored
KAFKA-20678: Exception handling for read call in ReplicaManagerLogReader.readAsync. (#22753)
The `read` call in `readAsync` was not protected by a try catch block which could throw an exception in a non-async fashion to the caller. This PR surrounds the call in try-catch and adds appropriate tests. Reviewers: Apoorv Mittal <apoorvmittal10@gmail.com>
1 parent 8c0ca4a commit 1db1ae7

2 files changed

Lines changed: 26 additions & 2 deletions

File tree

core/src/main/java/kafka/server/share/ReplicaManagerLogReader.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,8 +115,15 @@ public CompletableFuture<LinkedHashMap<TopicIdPartition, LogReadResult>> readAsy
115115
}
116116

117117
// Perform the local read for all partitions once; remote follow-ups (if any) are issued per partition.
118-
LinkedHashMap<TopicIdPartition, LogReadResult> localReadResults =
119-
read(fetchParams, partitionsToFetch, topicPartitionFetchOffsets, partitionMaxBytes);
118+
// read() is expected to convert per-partition failures into a LogReadResult carrying an error code
119+
// rather than throwing, but guard against an unexpected throw here anyway - readAsync must surface
120+
// failures via the returned future, not by throwing synchronously out of this method.
121+
LinkedHashMap<TopicIdPartition, LogReadResult> localReadResults;
122+
try {
123+
localReadResults = read(fetchParams, partitionsToFetch, topicPartitionFetchOffsets, partitionMaxBytes);
124+
} catch (Exception e) {
125+
return CompletableFuture.failedFuture(e);
126+
}
120127

121128
// Only look at partitions with non-null read results.
122129
LinkedHashMap<TopicIdPartition, CompletableFuture<LogReadResult>> futures = new LinkedHashMap<>();

core/src/test/java/kafka/server/share/ReplicaManagerLogReaderTest.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,23 @@ public void testReadAsyncReturnsErrorFromLocalRead() throws Exception {
381381
verify(replicaManager, never()).remoteLogManager();
382382
}
383383

384+
@Test
385+
public void testReadAsyncCompletesExceptionallyWhenLocalReadThrows() {
386+
ReplicaManager replicaManager = mockReplicaManager();
387+
RuntimeException readError = new RuntimeException("read failed");
388+
when(replicaManager.readFromLog(any(), any(), any(), anyBoolean())).thenThrow(readError);
389+
390+
ReplicaManagerLogReader logReader = new ReplicaManagerLogReader(replicaManager);
391+
CompletableFuture<LinkedHashMap<TopicIdPartition, LogReadResult>> future =
392+
logReader.readAsync(fetchParams(), Set.of(TOPIC_ID_PARTITION),
393+
offsets(TOPIC_ID_PARTITION), maxBytes(TOPIC_ID_PARTITION), true);
394+
395+
// readAsync must surface the failure via the returned future rather than throwing synchronously.
396+
assertTrue(future.isCompletedExceptionally());
397+
ExecutionException exception = assertThrows(ExecutionException.class, () -> future.get(10, TimeUnit.SECONDS));
398+
assertSame(readError, exception.getCause());
399+
}
400+
384401
@Test
385402
public void testReadAsyncReturnsNoneErrorWhenRemoteReadFailsButLocalSucceeds() throws Exception {
386403
ReplicaManager replicaManager = mockReplicaManager();

0 commit comments

Comments
 (0)