KAFKA-14473: Move AbstractIndex to storage module#13007
Conversation
ijuma
force-pushed
the
kafka-14473-move-abstract-index-to-storage
branch
3 times, most recently
from
049719f to
d686dda
Compare
ijuma
force-pushed
the
kafka-14473-move-abstract-index-to-storage
branch
from
d686dda to
0aab4d5
Compare
| /** | ||
| * The abstract index class which holds entry format agnostic methods. | ||
| */ | ||
| public abstract class AbstractIndex { |
There was a problem hiding this comment.
Earlier AbstractIndex is Closeable. But this java class is missing implementing Closeable.
abstract class AbstractIndex(@volatile private var _file: File, val baseOffset: Long, val maxIndexSize: Int = -1,
val writable: Boolean) extends Closeable
There was a problem hiding this comment.
Good catch, I'll fix this soon.
ijuma
force-pushed
the
kafka-14473-move-abstract-index-to-storage
branch
2 times, most recently
from
c68327a to
ea87980
Compare
ijuma
force-pushed
the
kafka-14473-move-abstract-index-to-storage
branch
from
ea87980 to
3033552
Compare
| else | ||
| return new BinarySearchResult(mid, mid); | ||
| } | ||
| int blah; |
There was a problem hiding this comment.
Instead of defining blah, could we just reuse hi?
There was a problem hiding this comment.
My bad, I meant to rename this, but missed it. Your suggestion sounds good.
| } | ||
| } | ||
|
|
||
| // GuardedBy `lock` |
There was a problem hiding this comment.
Should we change the comment to the following?
The caller is expected to hold lock when calling this method.
There was a problem hiding this comment.
I wanted to use the @GuardedBy annotation, but there are some licensing issues with the jcip jar (LGPL). I'll go with your suggested comment for now.
storage/src/main/java/org/apache/kafka/server/log/internals/TimeIndex.java
|
@junrao Thanks for the review, I addressed the comments. |
|
Test failures are unrelated.
|
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>
3 participants
For broader context on this change, please check:
Committer Checklist (excluded from commit message)