KAFKA-18180: Move OffsetResultHolder to storage module #18100

m1a2st · 2024-12-08T17:00:30Z

Jira: https://issues.apache.org/jira/browse/KAFKA-18180

as title

Committer Checklist (excluded from commit message)

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

m1a2st · 2024-12-08T17:03:33Z

storage/src/main/java/org/apache/kafka/storage/log/OffsetResultHolder.java

+        return result;
+    }
+
+    public static class FileRecordsOrError {


I don't think this name is precise enough, but I can't think of a better idea right now.

clolov

I carried out an initial pass - thank you for the changes!

clolov · 2024-12-10T11:15:00Z

core/src/main/java/kafka/log/remote/RemoteLogOffsetReader.java

        try {
            // If it is not found in remote storage, then search in the local storage starting with local log start offset.
            Option<FileRecords.TimestampAndOffset> timestampAndOffsetOpt =
                    OptionConverters.toScala(rlm.findOffsetByTimestamp(tp, timestamp, startingOffset, leaderEpochCache))
                    .orElse(searchInLocalLog::get);
-            result = Right.apply(timestampAndOffsetOpt);
+            result = new OffsetResultHolder.FileRecordsOrError(Optional.empty(),
+                    timestampAndOffsetOpt.isDefined() ? Optional.of(timestampAndOffsetOpt.get()) : Optional.empty());


Can't timestampAndOffsetOpt.isDefined() ? Optional.of(timestampAndOffsetOpt.get()) : Optional.empty() be simplified to just timestampAndOffsetOpt?

Ah, I think I answered my own question - one is a Scala Option and the other is a Java Optional?

Still, maybe something like https://www.scala-lang.org/api/2.13.6/scala/jdk/OptionConverters$.html will simplify this?

Ah, I think I answered my own question - one is a Scala Option and the other is a Java Optional?

Yes, Scala Option and Java Optional is different type

Still, maybe something like https://www.scala-lang.org/api/2.13.6/scala/jdk/OptionConverters$.html will simplify this?

It a good idea, I will address it :)

clolov · 2024-12-10T11:16:21Z

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

      case ListOffsetsRequest.EARLIEST_TIMESTAMP | ListOffsetsRequest.EARLIEST_LOCAL_TIMESTAMP =>
        getOffsetByTimestamp
      case _ =>
        val offsetResultHolder = getOffsetByTimestamp
-        offsetResultHolder.maybeOffsetsError = maybeOffsetsError
-        offsetResultHolder.lastFetchableOffset = Some(lastFetchableOffset)
+        offsetResultHolder.maybeOffsetsError(if (maybeOffsetsError.isDefined) Optional.of(maybeOffsetsError.get) else Optional.empty())


Same question

clolov · 2024-12-10T11:23:44Z

storage/src/main/java/org/apache/kafka/storage/log/OffsetResultHolder.java

+        this.futureHolderOpt = futureHolderOpt;
+    }
+
+    public OffsetResultHolder(Optional<TimestampAndOffset> timestampAndOffsetOpt) {


Is there a downside to defining a few more constructors which take the concrete type and create an Optional internally? I believe this will save you a lot of Optional.of(new Something() elsewhere, no?

mimaison · 2024-12-10T15:14:00Z

I had opened a PR moving only AsyncOffsetReadFutureHolder in #18095

m1a2st · 2024-12-10T15:16:40Z

@mimaison, thanks for your reminder, this PR will wait for #18095 merge, and I will rebase on it

clolov · 2024-12-11T09:58:58Z

Heya @m1a2st, I reviewed, approved and merged Mickael's PR. If you rebase this PR on top of trunk, I can then review it as well

# Conflicts: # core/src/main/scala/kafka/log/OffsetResultHolder.scala # core/src/main/scala/kafka/server/ListOffsetsPartitionStatus.scala # core/src/test/scala/integration/kafka/server/DelayedRemoteListOffsetsTest.scala

m1a2st · 2024-12-11T11:42:14Z

Hello @clolov, I have been merge trunk into this PR, Thanks for your review :)

clolov

Thank you for the changes and apologies for the delay!

m1a2st · 2024-12-20T12:26:40Z

Thanks for @clolov and @mimaison review.

clolov · 2024-12-20T13:49:58Z

I believe there were failures in trunk after this build. Checking why and issuing a fix

m1a2st · 2024-12-20T13:51:38Z

There is the hotfix
#18284

clolov · 2024-12-20T13:52:42Z

Ah, thank you, reviewing!

…e-old-protocol-versions * apache-github/trunk: KAFKA-18312: Added entityType: topicName to SubscribedTopicNames in ShareGroupHeartbeatRequest.json (apache#18285) HOTFIX: fix incompatible types: Optional<TimestampAndOffset> cannot be converted to Option<TimestampAndOffset> (apache#18284) MINOR Fix some test-catalog issues (apache#18272) KAFKA-18180: Move OffsetResultHolder to storage module (apache#18100) KAFKA-18301; Make coordinator records first class citizen (apache#18261) KAFKA-18262 Remove DefaultPartitioner and UniformStickyPartitioner (apache#18204) KAFKA-18296 Remove deprecated KafkaBasedLog constructor (apache#18257) KAFKA-12829: Remove old Processor and ProcessorSupplier interfaces (apache#18238) KAFKA-18292 Remove deprecated methods of UpdateFeaturesOptions (apache#18245) KAFKA-12829: Remove deprecated Topology#addProcessor of old Processor API (apache#18154) KAFKA-18035, KAFKA-18306, KAFKA-18092: Address TransactionsTest flaky tests (apache#18264) MINOR: change the default linger time in the new coordinator (apache#18274) KAFKA-18305: validate controller.listener.names is not in inter.broker.listener.name for kcontrollers (apache#18222) KAFKA-18207: Serde for handling transaction records (apache#18136) KAFKA-13722: Refactor Kafka Streams store interfaces (apache#18243) KAFKA-17131: Refactor TimeDefinitions (apache#18241) MINOR: Fix MessageFormatters (apache#18266) Mark flaky tests for Dec 18, 2024 (apache#18263)

Reviewers: Christo Lolov <[email protected]>

ijuma · 2025-01-09T18:43:17Z

Fyi, I cherry-picked this to 4.0 as it made it easier to then cherry-pick #18321.

Reviewers: Christo Lolov <[email protected]>

m1a2st added 3 commits December 8, 2024 14:40

wip

f4945c6

transfer AsyncOffsetReadFutureHolder from scala to java

d5fcd0c

fix fail test

165a2cc

github-actions bot added core Kafka Broker storage Pull requests that target the storage module tiered-storage Related to the Tiered Storage feature KIP-932 Queues for Kafka labels Dec 8, 2024

m1a2st commented Dec 8, 2024

View reviewed changes

update the constructor check

89e3fd7

clolov reviewed Dec 10, 2024

View reviewed changes

addressed comments

59f7f6a

m1a2st added 3 commits December 11, 2024 18:52

delete AsyncOffsetReadFutureHolder before merge trunk

4894e5e

Merge branch 'trunk' into KAFKA-18180

b311ac5

# Conflicts: # core/src/main/scala/kafka/log/OffsetResultHolder.scala # core/src/main/scala/kafka/server/ListOffsetsPartitionStatus.scala # core/src/test/scala/integration/kafka/server/DelayedRemoteListOffsetsTest.scala

rebase trunk and resolve conflict

145f79f

fix fail test

a35fe88

chia7712 added the ci-approved label Dec 13, 2024

Merge branch 'trunk' into KAFKA-18180

435c336

clolov approved these changes Dec 20, 2024

View reviewed changes

clolov merged commit e8863c9 into apache:trunk Dec 20, 2024
9 checks passed

mumrah mentioned this pull request Dec 20, 2024

HOTFIX: fix incompatible types: Optional<TimestampAndOffset> cannot be converted to Option<TimestampAndOffset> #18284

Merged

3 tasks

tedyu pushed a commit to tedyu/kafka that referenced this pull request Jan 6, 2025

KAFKA-18180: Move OffsetResultHolder to storage module (apache#18100)

2876657

Reviewers: Christo Lolov <[email protected]>

ijuma pushed a commit that referenced this pull request Jan 9, 2025

KAFKA-18180: Move OffsetResultHolder to storage module (#18100)

0401f10

Reviewers: Christo Lolov <[email protected]>

KAFKA-18180: Move OffsetResultHolder to storage module #18100

KAFKA-18180: Move OffsetResultHolder to storage module #18100

Conversation

m1a2st commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

m1a2st Dec 8, 2024

Choose a reason for hiding this comment

Uh oh!

clolov left a comment

Choose a reason for hiding this comment

Uh oh!

clolov Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

clolov Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

clolov Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

m1a2st Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

clolov Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

clolov Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

mimaison commented Dec 10, 2024

Uh oh!

m1a2st commented Dec 10, 2024

Uh oh!

clolov commented Dec 11, 2024

Uh oh!

m1a2st commented Dec 11, 2024

Uh oh!

clolov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m1a2st commented Dec 20, 2024

Uh oh!

clolov commented Dec 20, 2024

Uh oh!

m1a2st commented Dec 20, 2024

Uh oh!

clolov commented Dec 20, 2024

Uh oh!

ijuma commented Jan 9, 2025

Uh oh!

Uh oh!

m1a2st commented Dec 8, 2024 •

edited

Loading