abhijeetk88 commented on code in PR #14363:
URL: https://github.com/apache/kafka/pull/14363#discussion_r1321437948
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java:
##########
@@ -523,7 +533,9 @@ public void markForCleanup() throws IOException {
markedForCleanup = true;
offsetIndex.renameTo(new
File(Utils.replaceSuffix(offsetIndex.file().getPath(), "",
LogFileUtils.DELETED_FILE_SUFFIX)));
timeIndex.renameTo(new
File(Utils.replaceSuffix(timeIndex.file().getPath(), "",
LogFileUtils.DELETED_FILE_SUFFIX)));
- txnIndex.renameTo(new
File(Utils.replaceSuffix(txnIndex.file().getPath(), "",
LogFileUtils.DELETED_FILE_SUFFIX)));
+ if (txnIndexOpt.isPresent()) {
+ txnIndexOpt.get().renameTo(new
File(Utils.replaceSuffix(txnIndexOpt.get().file().getPath(), "",
LogFileUtils.DELETED_FILE_SUFFIX)));
+ }
Review Comment:
I wanted to avoid doing this because the `renameTo` method throws
IOException and then we have to do it something like this, which looks ugly.
```
txnIndexOpt.ifPresent(transactionIndex -> {
try {
transactionIndex.renameTo(new
File(Utils.replaceSuffix(transactionIndex.file().getPath(), "",
LogFileUtils.DELETED_FILE_SUFFIX)));
} catch (IOException e) {
throw new RuntimeException(e);
}
});
```
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java:
##########
@@ -545,7 +557,9 @@ public void cleanup() throws IOException {
timeIndex.deleteIfExists();
return null;
}, () -> {
- txnIndex.deleteIfExists();
+ if (txnIndexOpt.isPresent()) {
+ txnIndexOpt.get().deleteIfExists();
+ }
Review Comment:
Same here. It would become something like this.
```
txnIndexOpt.ifPresent(transactionIndex -> {
try {
transactionIndex.deleteIfExists();
} catch (IOException e) {
throw new RuntimeException(e);
}
});
```
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java:
##########
@@ -573,8 +587,7 @@ public String toString() {
return "Entry{" +
"offsetIndex=" + offsetIndex.file().getName() +
", timeIndex=" + timeIndex.file().getName() +
- ", txnIndex=" + txnIndex.file().getName() +
- '}';
+ ", txnIndex=" + txnIndexOpt.map(index ->
index.file().getName()) + '}';
Review Comment:
If the index is present, it prints something like "Optional[filename]". I
think printing "Optional.empty", when empty is in line with the non-empty case
and I would prefer it this way.
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java:
##########
@@ -382,6 +388,9 @@ private RemoteIndexCache.Entry
createCacheEntry(RemoteLogSegmentMetadata remoteL
TransactionIndex txnIndex = loadIndexFile(txnIndexFile,
remoteLogSegmentMetadata, rlsMetadata -> {
try {
return remoteStorageManager.fetchIndex(rlsMetadata,
IndexType.TRANSACTION);
+ } catch (RemoteResourceNotFoundException e) {
+ // Don't throw an exception since the transaction index
may not exist because of no transactional records.
+ return null;
Review Comment:
Do you mean we should add javadoc to the below method? This one is the
caller and the second parameter could receive null input stream.
```
private <T> T loadIndexFile(File file, RemoteLogSegmentMetadata
remoteLogSegmentMetadata,
Function<RemoteLogSegmentMetadata,
InputStream> fetchRemoteIndex,
Function<File, T> readIndex) throws
IOException {
```
The method is private. Should we still add a javadoc?
--
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
