Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KAFKA-14475: Move TimeIndex/LazyIndex to storage module #13010

Merged
merged 3 commits into from Dec 21, 2022
Merged

KAFKA-14475: Move TimeIndex/LazyIndex to storage module #13010

merged 3 commits into from Dec 21, 2022

Conversation

ijuma
Copy link
Contributor

@ijuma ijuma commented Dec 17, 2022 •
edited

For broader context on this change, please check:

Committer Checklist (excluded from commit message)

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

@ijuma ijuma force-pushed the kafka-14475-move-time-index-to-storage branch from 04bca37 to 3beae7d Compare December 17, 2022 20:30
@ijuma ijuma mentioned this pull request Dec 17, 2022
Merged
3 tasks
satishd
satishd reviewed Dec 19, 2022
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ijuma for the PR. Do you plan to move LazyIndex to storage module as part of this change?

@ijuma
Copy link
Contributor Author

ijuma commented Dec 19, 2022

Yes, I'll push that commit soon. Feel free to help review the transactions index one (first unmerged in the series) if you have cycles. :)

@ijuma ijuma force-pushed the kafka-14475-move-time-index-to-storage branch 3 times, most recently from 69d4d24 to 720cc07 Compare December 19, 2022 20:17
@ijuma
Copy link
Contributor Author

ijuma commented Dec 19, 2022

@satishd I pushed the LazyIndex commit too. Note that I haven't verified if all tests pass yet. I'll check the CI build later and fix any issues that come up.

@ijuma ijuma force-pushed the kafka-14475-move-time-index-to-storage branch 2 times, most recently from bb15456 to e094575 Compare December 20, 2022 04:12
@ijuma ijuma marked this pull request as ready for review December 20, 2022 04:13
@ijuma
Copy link
Contributor Author

ijuma commented Dec 20, 2022

@satishd This should be ready for review now.

satishd
satishd approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ijuma for the PR and adding LazyIndex related changes also. LGTM.

@ijuma
Copy link
Contributor Author

ijuma commented Dec 20, 2022

Test failures look unrelated. Re-running the tests just in case.

@ijuma ijuma force-pushed the kafka-14475-move-time-index-to-storage branch from e094575 to 9294e30 Compare December 20, 2022 19:55
junrao
junrao approved these changes Dec 20, 2022
View reviewed changes
Copy link
Contributor

@junrao junrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ijuma : Thanks for the PR. LGTM. Just a minor comment.

storage/src/main/java/org/apache/kafka/server/log/internals/TimeIndex.java Outdated

private volatile TimestampOffset lastEntry;

public TimeIndex(File file, long baseOffset) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This constructor seems unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I think I changed some callers not to rely on this anymore, so we can remove it. Pushed the fix.

@ijuma
Copy link
Contributor Author

ijuma commented Dec 21, 2022

JDK 8 and JDK 17 builds passed, JDK 11 failures are unrelated:

Build / JDK 11 and Scala 2.13 / kafka.api.TransactionsTest.testBumpTransactionalEpoch(String).quorum=kraft 1 min 18 sec 1
Build / JDK 11 and Scala 2.13 / org.apache.kafka.streams.processor.internals.DefaultStateUpdaterTest.shouldRemovePausedAndUpdatingTasksOnShutdown()

@ijuma ijuma merged commit c4f1036 into apache:trunk Dec 21, 2022
@ijuma ijuma deleted the kafka-14475-move-time-index-to-storage branch December 21, 2022 03:08
guozhangwang pushed a commit to guozhangwang/kafka that referenced this pull request Jan 25, 2023
@ijuma @guozhangwang
KAFKA-14475: Move TimeIndex/LazyIndex to storage module (apache#13010)
c6bdbc2 
For broader context on this change, please check:

* KAFKA-14470: Move log layer to storage module

 Reviewers: Jun Rao <junrao@gmail.com>, Satish Duggana <satishd@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@junrao junrao junrao approved these changes

@satishd satishd satishd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@ijuma @junrao @satishd