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

KAFKA-17757: Remove Utils.mkEntry #17488

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

mingyen066
Copy link
Contributor

@mingyen066 mingyen066 commented Oct 13, 2024

Updates: Since there are still many tests using mkEntry to create entries with null values, this PR will migrate all the code that can be converted to use Map.entry. The remaining code that still uses mkEntry will only for creating null entries.

Removes Utils#mkMap and Utils#mkEntry as JDK 9+ allows Map.ofEntries and Map.entry for creating immutable map.

Utils#mkMap will be handled in KAFKA-17820

Committer Checklist (excluded from commit message)

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

@github-actions github-actions bot added streams core Kafka Broker consumer connect kraft storage Pull requests that target the storage module KIP-932 Queues for Kafka clients labels Oct 13, 2024
@mingyen066 mingyen066 changed the title KAFKA-17759 Remove Utils.mkMap and Utils.mkEntry KAFKA-17759: Remove Utils.mkMap and Utils.mkEntry Oct 13, 2024
@mingyen066 mingyen066 changed the title KAFKA-17759: Remove Utils.mkMap and Utils.mkEntry [Draft] KAFKA-17759: Remove Utils.mkMap and Utils.mkEntry Oct 13, 2024
@mingyen066 mingyen066 marked this pull request as ready for review October 13, 2024 13:29
@mingyen066 mingyen066 changed the title [Draft] KAFKA-17759: Remove Utils.mkMap and Utils.mkEntry [Draft] KAFKA-17757: Remove Utils.mkMap and Utils.mkEntry Oct 17, 2024
@mingyen066 mingyen066 changed the title [Draft] KAFKA-17757: Remove Utils.mkMap and Utils.mkEntry [Draft] KAFKA-17757: Remove Utils.mkEntry Oct 17, 2024
@mingyen066 mingyen066 changed the title [Draft] KAFKA-17757: Remove Utils.mkEntry KAFKA-17757: Remove Utils.mkEntry Oct 17, 2024
@chia7712
Copy link
Contributor

@mingyen066 Please check the CI

@@ -94,8 +93,8 @@ public void testFlushFailureWhenWriteToSecondaryStoreFailsForTombstoneOffsets()
AtomicReference<Throwable> callbackError = new AtomicReference<>();

Future<Void> setFuture = offsetBackingStore.set(getSerialisedOffsets(mkMap(
mkEntry(OFFSET_KEY_SERIALIZED, null),
Copy link
Contributor

Choose a reason for hiding this comment

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

this case needs to have map carrying null value, so you can add helper for this case specifically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since there are still many tests using mkEntry to create entries with null values, this PR will migrate all the code that can be converted to use Map.entry. The remaining code that still uses mkEntry will only for creating null entries.

@AndrewJSchofield AndrewJSchofield removed the KIP-932 Queues for Kafka label Oct 28, 2024
@github-actions github-actions bot added the KIP-932 Queues for Kafka label Oct 30, 2024
@chia7712
Copy link
Contributor

@mingyen066 please fix conflicts

@mingyen066 mingyen066 force-pushed the KAFKA-17758-Replace-Utils.mkMap-and-UtilsmkEntry-with-Map.entry-and-Map.ofEntries branch from 5318b5b to b453825 Compare November 26, 2024 19:37
@github-actions github-actions bot added the tools label Nov 26, 2024
@mingyen066
Copy link
Contributor Author

Hi @chia7712, sorry for the late reply. I've migrated all the mkEntry usages to Map.entry, except for those used to create entries with null values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci-approved clients connect consumer core Kafka Broker KIP-932 Queues for Kafka kraft storage Pull requests that target the storage module streams tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants