You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "sjhajharia (via GitHub)" <gi...@apache.org> on 2024/06/05 08:23:08 UTC
[PR] MINOR: Cleanup storage module [kafka]
sjhajharia opened a new pull request, #16207:
URL: https://github.com/apache/kafka/pull/16207
Cleanups in Storage Module
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16207:
URL: https://github.com/apache/kafka/pull/16207#issuecomment-2210248844
@sjhajharia Could you please rebase code to trigger QA again? There are two uncompleted suits
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on PR #16207:
URL: https://github.com/apache/kafka/pull/16207#issuecomment-2210244090
Hey @chia7712, could you PTAL at this one final time
Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "jlprat (via GitHub)" <gi...@apache.org>.
jlprat commented on PR #16207:
URL: https://github.com/apache/kafka/pull/16207#issuecomment-2260721446
Triggering CI again as the build was aborted when running the JDK8 pipeline
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on PR #16207:
URL: https://github.com/apache/kafka/pull/16207#issuecomment-2151470915
Thanks @chia7712 for the review. Addressed the comments in the new commit. Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16207:
URL: https://github.com/apache/kafka/pull/16207#discussion_r1628201256
##########
storage/src/test/java/org/apache/kafka/server/log/remote/storage/RemoteTopicPartitionDirectory.java:
##########
@@ -95,7 +95,7 @@ boolean delete() {
void traverse(final LocalTieredStorageTraverser traverser) {
traverser.visitTopicIdPartition(topicIdPartition);
- listFilesets().stream().forEach(fileset -> traverser.visitSegment(fileset));
+ listFilesets().forEach(fileset -> traverser.visitSegment(fileset));
Review Comment:
how about: `listFilesets().forEach(traverser::visitSegment);`
##########
storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java:
##########
@@ -176,14 +176,14 @@ public Optional<EpochEntry> latestEntry() {
*/
public OptionalInt latestEpoch() {
Optional<EpochEntry> entry = latestEntry();
- return entry.isPresent() ? OptionalInt.of(entry.get().epoch) : OptionalInt.empty();
+ return entry.map(epochEntry -> OptionalInt.of(epochEntry.epoch)).orElseGet(OptionalInt::empty);
}
public OptionalInt previousEpoch() {
lock.readLock().lock();
try {
Optional<Map.Entry<Integer, EpochEntry>> lowerEntry = latestEntry().flatMap(entry -> Optional.ofNullable(epochs.lowerEntry(entry.epoch)));
- return lowerEntry.isPresent() ? OptionalInt.of(lowerEntry.get().getKey()) : OptionalInt.empty();
+ return lowerEntry.map(integerEpochEntryEntry -> OptionalInt.of(integerEpochEntryEntry.getKey())).orElseGet(OptionalInt::empty);
Review Comment:
maybe we can eliminate local variable `lowerEntry`
```java
return latestEntry().flatMap(entry -> Optional.ofNullable(epochs.lowerEntry(entry.epoch)))
.map(entry -> OptionalInt.of(entry.getKey()))
.orElseGet(OptionalInt::empty);
```
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/LogSegment.java:
##########
@@ -459,9 +459,7 @@ public FetchDataInfo read(long startOffset, int maxSize, Optional<Long> maxPosit
public OptionalLong fetchUpperBoundOffset(OffsetPosition startOffsetPosition, int fetchSize) throws IOException {
Optional<OffsetPosition> position = offsetIndex().fetchUpperBoundOffset(startOffsetPosition, fetchSize);
- if (position.isPresent())
- return OptionalLong.of(position.get().offset);
- return OptionalLong.empty();
+ return position.map(offsetPosition -> OptionalLong.of(offsetPosition.offset)).orElseGet(OptionalLong::empty);
Review Comment:
ditto
```java
return offsetIndex().fetchUpperBoundOffset(startOffsetPosition, fetchSize)
.map(offsetPosition -> OptionalLong.of(offsetPosition.offset)).orElseGet(OptionalLong::empty);
```
##########
storage/src/main/java/org/apache/kafka/storage/internals/epoch/LeaderEpochFileCache.java:
##########
@@ -176,14 +176,14 @@ public Optional<EpochEntry> latestEntry() {
*/
public OptionalInt latestEpoch() {
Optional<EpochEntry> entry = latestEntry();
- return entry.isPresent() ? OptionalInt.of(entry.get().epoch) : OptionalInt.empty();
+ return entry.map(epochEntry -> OptionalInt.of(epochEntry.epoch)).orElseGet(OptionalInt::empty);
Review Comment:
ditto
```java
latestEntry().map(epochEntry -> OptionalInt.of(epochEntry.epoch)).orElseGet(OptionalInt::empty);
```
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16207:
URL: https://github.com/apache/kafka/pull/16207
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16207:
URL: https://github.com/apache/kafka/pull/16207#discussion_r1629837184
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/SkimpyOffsetMap.java:
##########
@@ -109,7 +109,7 @@ public long get(ByteBuffer key) throws DigestException {
hashInto(key, hash1);
// search for the hash of this key by repeated probing until we find the hash we are looking for or we find an empty slot
int attempt = 0;
- int pos = 0;
+ int pos;
Review Comment:
maybe we can move `int pos;` into the loop?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Cleanup storage module [kafka]
Posted by "sjhajharia (via GitHub)" <gi...@apache.org>.
sjhajharia commented on code in PR #16207:
URL: https://github.com/apache/kafka/pull/16207#discussion_r1630640609
##########
storage/src/main/java/org/apache/kafka/storage/internals/log/SkimpyOffsetMap.java:
##########
@@ -109,7 +109,7 @@ public long get(ByteBuffer key) throws DigestException {
hashInto(key, hash1);
// search for the hash of this key by repeated probing until we find the hash we are looking for or we find an empty slot
int attempt = 0;
- int pos = 0;
+ int pos;
Review Comment:
Makes sense. Moved the same in the next commit.
Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org