KAFKA-14522 Rewrite/Move of RemoteIndexCache to storage module. #13275

satishd · 2023-02-18T11:32:00Z

KAFKA-14522 Rewrite/Move of RemoteIndexCache to the storage module.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

showuon

Had an early review, left some comments. Thanks.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

satishd · 2023-03-01T09:10:45Z

Thanks @showuon for your review. Addressed the comments with the latest commit.

junrao

@satishd : Thanks for the PR. A few comments below.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

junrao · 2023-02-27T18:59:52Z

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

+                    try {
+                        entry.markForCleanup();
+                    } catch (IOException e) {
+                        throw new KafkaException(e);


The constructor declares throwing IOException. Why do we need to convert IOException to KafkaException? Ditto in other methods like init()?

This exception is not directly thrown to the constructor. It is inside an anonymous inner class of LinkedHashMap#removeEldestEntry. This method is not declared with IOException.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

satishd · 2023-03-02T17:24:55Z

Thanks @junrao for your review. Addressed your comments inline and/or updated with the latest commit.

junrao

@satishd : Thanks for the updated PR. A few more comments.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java

satishd · 2023-07-04T06:38:51Z

@junrao @showuon Addressed the review comments with the latest commits. It also includes the recent changes on RemoteIndexCache in trunk.

showuon · 2023-07-05T07:59:19Z

Will take a look this week or next week. Thanks.

showuon

@satishd , thanks for the update. Left some comments.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

junrao

@satishd : Thanks for the updated PR. Just a few minor comments.

junrao · 2023-07-06T23:01:16Z

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

+        cleanerThread.start();
+    }
+
+    public LinkedBlockingQueue<Entry> expiredIndexes() {


This and the next two methods are only used for tests. Should we expose them at the package level?

Tests are written at different package level in core module. We can revisit this once the tests are also moved to java and the respective module/package.

junrao · 2023-07-06T23:10:51Z

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

+                            log.error("Cleaner thread received interruption but remote index cache is not closed", ex);
+                            throw new KafkaException(ex);
+                        } else {
+                            log.debug("Cleaner thread was interrupted on cache shutdown");


ShutdownableThread already logs the shutdown.

This comment is very specific to the interrupted scenario.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

junrao · 2023-07-07T18:20:29Z

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

+        // Delete any .deleted files remained from the earlier run of the broker.
+        try (Stream<Path> paths = Files.list(cacheDir.toPath())) {
+            paths.forEach(path -> {
+                if (path.endsWith(LogFileUtils.DELETED_FILE_SUFFIX)) {


Should we clean up the tmp file too?

build.gradle

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

Cleanedup index file suffix usages and other minor cleaups

….scala Other minor changes

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

satishd · 2023-07-10T05:39:34Z

Thanks @junrao for the review comment. Addressed them with the latest commits and/or inline.

satishd · 2023-07-10T05:42:02Z

Thanks @divijvaidya for the review. Addressed them with inline and/or with the latest commits.

showuon

LGTM!

divijvaidya

Left a last round of minor comments. I am good to ship this PR once they are resolved.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

satishd · 2023-07-10T15:38:51Z

Thanks @divijvaidya for the latest review. Addressed them with the latest commit.

jeqo

Just a few comments related to TxnIndex potentially being optional.

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java

satishd · 2023-07-11T03:18:12Z

Just a few comments related to TxnIndex potentially being optional.

@jeqo There is already KAFKA-14993 to address that. @kamalcph was working on that. I did not want to add those changes to this PR as it is tracked separately.

jeqo

Sure, thanks for the reminder. LGTM, thanks @satishd !

kamalcph

LGTM

junrao

@satishd : Thanks for the updated PR. LGTM

satishd · 2023-07-11T18:21:14Z

A few tests are failed but those are not related to the changes in the PR.

…he#13275) KAFKA-14522 Rewrite and Move of RemoteIndexCache to storage module. Cleanedup index file suffix usages and other minor cleanups Reviewers: Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk>, Luke Chen <showuon@gmail.com>, Divij Vaidya <diviv@amazon.com>, Kamal Chandraprakash<kamal.chandraprakash@gmail.com>, Jorge Esteban Quilcate Otoya <quilcate.jorge@gmail.com>

showuon reviewed Feb 19, 2023

View reviewed changes

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java Outdated Show resolved Hide resolved

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java Outdated Show resolved Hide resolved

satishd force-pushed the KAFKA-14522 branch 2 times, most recently from da17d44 to 90b914c Compare February 19, 2023 15:19

satishd marked this pull request as ready for review February 25, 2023 12:14

satishd requested review from showuon and junrao February 25, 2023 13:18

showuon reviewed Mar 1, 2023

View reviewed changes

storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java Outdated Show resolved Hide resolved

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java Outdated Show resolved Hide resolved

satishd force-pushed the KAFKA-14522 branch 2 times, most recently from 81ceeee to e49a357 Compare March 1, 2023 06:57

junrao reviewed Mar 2, 2023

View reviewed changes

satishd force-pushed the KAFKA-14522 branch from c5434dc to 4f3da7f Compare March 5, 2023 03:18

satishd requested a review from junrao March 5, 2023 05:09

junrao reviewed Mar 8, 2023

View reviewed changes

ijuma reviewed Mar 9, 2023

View reviewed changes

storage/src/main/java/org/apache/kafka/storage/internals/log/LogFileUtils.java Outdated Show resolved Hide resolved

satishd force-pushed the KAFKA-14522 branch from 4f3da7f to 3361ee3 Compare March 27, 2023 16:29

divijvaidya added the tiered-storage Pull requests associated with KIP-405 (Tiered Storage) label Jun 16, 2023

satishd force-pushed the KAFKA-14522 branch 2 times, most recently from b290f00 to 22a9c72 Compare July 4, 2023 06:14

satishd requested review from junrao and showuon July 4, 2023 06:39

showuon reviewed Jul 6, 2023

View reviewed changes

junrao reviewed Jul 6, 2023

View reviewed changes

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java Show resolved Hide resolved

divijvaidya mentioned this pull request Jul 7, 2023

KAFKA-15157: Print startup time for RemoteIndexCache #13970

Merged

3 tasks

junrao reviewed Jul 7, 2023

View reviewed changes

divijvaidya reviewed Jul 8, 2023

View reviewed changes

KAFKA-14522 Rewrite and Move of RemoteIndexCache to storage module.

52e299c

Cleanedup index file suffix usages and other minor cleaups

satishd added 5 commits July 9, 2023 10:21

Addressed review comments

e8ffe67

Refactored with the latest changes from trunk

14a35c1

Addressed review comments

119e88a

Addressed review comments

8105464

Pulled the changes from the trunk that were made in RemotreIndexCache…

2d8271e

….scala Other minor changes

satishd force-pushed the KAFKA-14522 branch from 48f5949 to 2d8271e Compare July 9, 2023 07:37

Addressed reveiw comments

38f7fbf

divijvaidya reviewed Jul 9, 2023

View reviewed changes

storage/src/main/java/org/apache/kafka/storage/internals/log/RemoteIndexCache.java Show resolved Hide resolved

Addressed reveiw comments

2d9a606

satishd force-pushed the KAFKA-14522 branch from 4a1e276 to 2d9a606 Compare July 10, 2023 04:31

showuon approved these changes Jul 10, 2023

View reviewed changes

divijvaidya reviewed Jul 10, 2023

View reviewed changes

Addressed review comments

40bdf14

divijvaidya approved these changes Jul 10, 2023

View reviewed changes

jeqo reviewed Jul 10, 2023

View reviewed changes

jeqo approved these changes Jul 11, 2023

View reviewed changes

kamalcph approved these changes Jul 11, 2023

View reviewed changes

satishd requested a review from junrao July 11, 2023 08:55

junrao approved these changes Jul 11, 2023

View reviewed changes

satishd merged commit 7e2f878 into apache:trunk Jul 11, 2023
1 check failed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14522 Rewrite/Move of RemoteIndexCache to storage module. #13275

KAFKA-14522 Rewrite/Move of RemoteIndexCache to storage module. #13275

satishd commented Feb 18, 2023

showuon left a comment

satishd commented Mar 1, 2023

junrao left a comment

junrao Feb 27, 2023

satishd Mar 2, 2023

satishd commented Mar 2, 2023

junrao left a comment

satishd commented Jul 4, 2023

showuon commented Jul 5, 2023

showuon left a comment

junrao left a comment

junrao Jul 6, 2023

satishd Jul 10, 2023 •

edited

junrao Jul 6, 2023

satishd Jul 10, 2023

junrao Jul 7, 2023

satishd commented Jul 10, 2023

satishd commented Jul 10, 2023

showuon left a comment

divijvaidya left a comment

satishd commented Jul 10, 2023

jeqo left a comment

satishd commented Jul 11, 2023

jeqo left a comment

kamalcph left a comment

junrao left a comment

satishd commented Jul 11, 2023

KAFKA-14522 Rewrite/Move of RemoteIndexCache to storage module. #13275

KAFKA-14522 Rewrite/Move of RemoteIndexCache to storage module. #13275

Conversation

satishd commented Feb 18, 2023

Committer Checklist (excluded from commit message)

showuon left a comment

Choose a reason for hiding this comment

satishd commented Mar 1, 2023

junrao left a comment

Choose a reason for hiding this comment

junrao Feb 27, 2023

Choose a reason for hiding this comment

satishd Mar 2, 2023

Choose a reason for hiding this comment

satishd commented Mar 2, 2023

junrao left a comment

Choose a reason for hiding this comment

satishd commented Jul 4, 2023

showuon commented Jul 5, 2023

showuon left a comment

Choose a reason for hiding this comment

junrao left a comment

Choose a reason for hiding this comment

junrao Jul 6, 2023

Choose a reason for hiding this comment

satishd Jul 10, 2023 • edited

Choose a reason for hiding this comment

junrao Jul 6, 2023

Choose a reason for hiding this comment

satishd Jul 10, 2023

Choose a reason for hiding this comment

junrao Jul 7, 2023

Choose a reason for hiding this comment

satishd commented Jul 10, 2023

satishd commented Jul 10, 2023

showuon left a comment

Choose a reason for hiding this comment

divijvaidya left a comment

Choose a reason for hiding this comment

satishd commented Jul 10, 2023

jeqo left a comment

Choose a reason for hiding this comment

satishd commented Jul 11, 2023

jeqo left a comment

Choose a reason for hiding this comment

kamalcph left a comment

Choose a reason for hiding this comment

junrao left a comment

Choose a reason for hiding this comment

satishd commented Jul 11, 2023

satishd Jul 10, 2023 •

edited