KAFKA-14726 Move/rewrite of LogReadInfo, LogOffsetSnapshot, LogStartOffsetIncrementReason to storage module #13304

satishd · 2023-02-25T03:29:10Z

KAFKA-14726 Move/rewrite of LogReadInfo, LogOffsetSnapshot, LogStartOffsetIncrementReason to 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

Overall LGTM! Left some comments. Thanks.

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

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

showuon · 2023-02-26T07:13:16Z

core/src/main/scala/kafka/raft/KafkaMetadataLog.scala

@@ -202,7 +202,7 @@ final class KafkaMetadataLog private (
  }

  override def highWatermark: LogOffsetMetadata = {
-    val LogOffsetSnapshot(_, _, hwm, _) = log.fetchOffsetSnapshot
+    val hwm = log.fetchOffsetSnapshot.highWatermark


nice refactor!

showuon · 2023-02-26T07:13:59Z

core/src/main/scala/kafka/log/UnifiedLog.scala

-      shallowMessageCount, validBytesCount, monotonic, lastOffsetOfFirstBatch)
+      shallowMessageCount, validBytesCount, monotonic, lastOffsetOfFirstBatch, Collections.emptyList[RecordError], null,
+      LeaderHwChange.NONE)


Why do we need to explicitly set default value here?

Earlier, we had default values in Scala. These changes did not introduce constructor overloading for different combinations as the usages were low. It is simpler to have a constructor and set this value from a couple of places.

satishd · 2023-02-27T08:39:37Z

Thanks @showuon for your review. Addressed them with inline replies or the latest commits.

junrao

@satishd : Thanks for the PR. A couple of comments below.

core/src/main/scala/kafka/cluster/Partition.scala

junrao · 2023-02-27T18:12:10Z

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

+/**
+ * Structure used for lower level reads using {@link kafka.cluster.Partition#fetchRecords()}.
+ */
+public class LogReadInfo {


All scala case classes define equal() and hashcode(). Do we need to define those explicitly in java?

Note that case class is used for many reasons. Do we actually need equals/hashCode (eg are these classes used as map keys or compared against each other)? If not, then it's maintenance overhead without benefit.

Right. This is the reason I did not add it earlier. But I am not strongly opinionated about that.
I am fine adding them with the latest commit.

satishd · 2023-02-28T17:32:49Z

Thanks @junrao for your review. Addressed them with the latest commit.

junrao

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

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

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

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

…tReason to storage module

satishd · 2023-03-01T16:18:03Z

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

junrao

@satishd : Thanks for the updated. One more comment.

junrao · 2023-03-02T19:01:50Z

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

+
+        return firstEntryIncomplete != that.firstEntryIncomplete &&
+                Objects.equals(fetchOffsetMetadata, that.fetchOffsetMetadata) &&
+                Objects.equals(records, that.records) &&


Hmm, FileRecords doesn't implement equals. So, maybe LogReadInfo should never be used for comparison?

+1, we can leave LogReadInfo not to implement equals/hashCode as it is not really needed with the current usages.

What's the point of having these methods implemented though? For example, what does it mean to compare a list of aborted transactions? Unless there are clear semantics, we're better off relying on reference equality only.

- Removed equals and hashCode methods from LogReadInfo and FetchDataInfo

junrao

@satishd : Thanks for the updated PR. LGTM

satishd force-pushed the KAFKA-14726 branch from a07100c to 083760c Compare February 25, 2023 09:10

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

satishd requested review from junrao, showuon and ijuma February 25, 2023 12:05

showuon reviewed Feb 26, 2023

View reviewed changes

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

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

showuon reviewed Feb 26, 2023

View reviewed changes

mimaison approved these changes Feb 27, 2023

View reviewed changes

junrao reviewed Feb 27, 2023

View reviewed changes

junrao reviewed Feb 28, 2023

View reviewed changes

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

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

ijuma reviewed Feb 28, 2023

View reviewed changes

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

satishd added 4 commits March 1, 2023 14:11

KAFKA-14726 Move/rewrite LogReadInfo to storage module.

6f46596

KAFKA-14726 Move/rewrite of LogOffsetSnapshot, LogStartOffsetIncremen…

a27a0f4

…tReason to storage module

Addressed review comments

2260691

Addressed review comments

6f17564

satishd force-pushed the KAFKA-14726 branch from f9349cc to 6f17564 Compare March 1, 2023 09:09

satishd added 2 commits March 1, 2023 15:36

Addressed review comments

7d1fcfd

Addressed review comments

8e5b7d2

showuon approved these changes Mar 2, 2023

View reviewed changes

junrao approved these changes Mar 2, 2023

View reviewed changes

Addressed review comments

2c7eb7c

- Removed equals and hashCode methods from LogReadInfo and FetchDataInfo

satishd requested a review from junrao March 3, 2023 15:40

junrao approved these changes Mar 3, 2023

View reviewed changes

satishd merged commit 97105a8 into apache:trunk Mar 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-14726 Move/rewrite of LogReadInfo, LogOffsetSnapshot, LogStartOffsetIncrementReason to storage module #13304

KAFKA-14726 Move/rewrite of LogReadInfo, LogOffsetSnapshot, LogStartOffsetIncrementReason to storage module #13304

satishd commented Feb 25, 2023

showuon left a comment

showuon Feb 26, 2023

showuon Feb 26, 2023

satishd Feb 27, 2023

satishd commented Feb 27, 2023

junrao left a comment

junrao Feb 27, 2023

ijuma Feb 28, 2023 •

edited

satishd Mar 1, 2023 •

edited

satishd commented Feb 28, 2023

junrao left a comment

satishd commented Mar 1, 2023

junrao left a comment

junrao Mar 2, 2023

satishd Mar 3, 2023

ijuma Mar 3, 2023

junrao left a comment

KAFKA-14726 Move/rewrite of LogReadInfo, LogOffsetSnapshot, LogStartOffsetIncrementReason to storage module #13304

KAFKA-14726 Move/rewrite of LogReadInfo, LogOffsetSnapshot, LogStartOffsetIncrementReason to storage module #13304

Conversation

satishd commented Feb 25, 2023

Committer Checklist (excluded from commit message)

showuon left a comment

Choose a reason for hiding this comment

showuon Feb 26, 2023

Choose a reason for hiding this comment

showuon Feb 26, 2023

Choose a reason for hiding this comment

satishd Feb 27, 2023

Choose a reason for hiding this comment

satishd commented Feb 27, 2023

junrao left a comment

Choose a reason for hiding this comment

junrao Feb 27, 2023

Choose a reason for hiding this comment

ijuma Feb 28, 2023 • edited

Choose a reason for hiding this comment

satishd Mar 1, 2023 • edited

Choose a reason for hiding this comment

satishd commented Feb 28, 2023

junrao 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 Mar 2, 2023

Choose a reason for hiding this comment

satishd Mar 3, 2023

Choose a reason for hiding this comment

ijuma Mar 3, 2023

Choose a reason for hiding this comment

junrao left a comment

Choose a reason for hiding this comment

ijuma Feb 28, 2023 •

edited

satishd Mar 1, 2023 •

edited