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.

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

MINOR: Move Timer/TimingWheel to server-common #13820

Merged
merged 12 commits into from Jun 14, 2023

Conversation

dajac
Copy link
Contributor

@dajac dajac commented Jun 6, 2023

This patch rewrite Timer and the related classes in Java and moves them to server-common module. It is basically a one to one rewrite of the Scala code. Note that MockTimer is not moved as part of this patch. It will be done separately.

Committer Checklist (excluded from commit message)

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

@dajac dajac added the KIP-848 label Jun 6, 2023
@dajac dajac marked this pull request as ready for review June 7, 2023 18:40
@dajac dajac requested a review from mumrah June 9, 2023 07:12
@dajac dajac requested a review from divijvaidya June 13, 2023 14:31
@dajac
Copy link
Contributor Author

dajac commented Jun 13, 2023

@divijvaidya As you started reviewing it, are you interested in finishing the review? That would unblock me.

@divijvaidya
Copy link
Contributor

Hi @dajac , yes let me take a look first thing tomorrow.

Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Reviewed the non test files so far. Leaving early feedback while I look at the test files.

Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Test files look good. One minor comment.

@dajac
Copy link
Contributor Author

dajac commented Jun 14, 2023

@divijvaidya Thanks for your review. I have addressed all your comments.

Copy link
Contributor

@divijvaidya divijvaidya left a comment

Choose a reason for hiding this comment

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

Looks good to me! The code is consistent with the scala implementation. I haven't reviewed for logical bugs but that is out of scope of this PR.

(waiting for CI test run to complete before approving)

@dajac
Copy link
Contributor Author

dajac commented Jun 14, 2023

Failed tests seem unrelated:

Build / JDK 11 and Scala 2.13 / testReplication() – org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest
1m 45s
Build / JDK 11 and Scala 2.13 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationBaseTest
1m 2s
Build / JDK 11 and Scala 2.13 / testRestartReplication() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest
3m 8s
Build / JDK 11 and Scala 2.13 / testReplicateSourceDefault() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest
1m 29s
Build / JDK 11 and Scala 2.13 / testDescribeAtMinIsrPartitions(String).quorum=zk – kafka.admin.TopicCommandIntegrationTest
6s
Build / JDK 11 and Scala 2.13 / [1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
14s
Build / JDK 8 and Scala 2.12 / testBumpTransactionalEpoch(String).quorum=kraft – kafka.api.TransactionsTest
1m 20s
Build / JDK 8 and Scala 2.12 / [1] Type=ZK, Name=testNewAndChangedTopicsInDualWrite, MetadataVersion=3.4-IV0, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
15s
Build / JDK 17 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
13s
Build / JDK 17 and Scala 2.13 / shouldWriteLatestOffsetsToCheckpointOnShutdown[exactly_once_v2] – org.apache.kafka.streams.integration.EosIntegrationTest

@dajac dajac merged commit 45a279e into apache:trunk Jun 14, 2023
1 check failed
@dajac dajac deleted the minor-java-timingwheel branch June 14, 2023 16:21
dajac added a commit that referenced this pull request Jul 6, 2023
This patch rewrites MockTimer in Java and moves it from core to server-common. This continues the work started in #13820.

Reviewers: Divij Vaidya <diviv@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants