Skip to content

feat: Add support for message keys in typed producer - #88

Closed
rohits1101 wants to merge 7 commits into
mainfrom
rohit/acq-3540-add-support-for-message-key-in-kp-kafka-producer
Closed

feat: Add support for message keys in typed producer#88
rohits1101 wants to merge 7 commits into
mainfrom
rohit/acq-3540-add-support-for-message-key-in-kp-kafka-producer

Conversation

@rohits1101

@rohits1101 rohits1101 commented Oct 16, 2023

Copy link
Copy Markdown

Pull Request Submission Checklist

Please confirm that you have done the following before requesting reviews:

  • I have confirmed that the PR type is appropriate for the change I am making according to
    the Honest Pull Request and Commit Message Naming Conventions.
  • I have typed an adequate description that explains why I am making this change.
  • I have installed and run standard pre-commit hooks that lints and validates my code.

Description

  • The kafka producer API does not support setting message keys
  • In our kafka-infrastructure, by default the topics are set to compat on keys
  • Having empty keys deletes the messages on the topic
  • To solve this, we need to do one of the two:
    1. Change the topic compaction policy (to not delete, to not compact on message keys)
    2. Change the KP interface to produce the messages with keys
  • We can go ahead with (2) because it also aligns with the organization wide convention of (1) topic deletion policy, (2) messages between acq, and mlops having their keys as application-status-ids

AC

  • Update the producer interface to also accept the keys, and produce the messages with message keys

This change is Reviewable

@rohits1101
rohits1101 requested a review from a team as a code owner October 16, 2023 07:19
@rohits1101
rohits1101 requested a review from misranitin October 16, 2023 07:19
@rohits1101
rohits1101 force-pushed the rohit/acq-3540-add-support-for-message-key-in-kp-kafka-producer branch from 31c6514 to 20a0400 Compare October 16, 2023 07:30
@pull-request-size pull-request-size Bot added size/M and removed size/S labels Oct 16, 2023
@rohits1101
rohits1101 force-pushed the rohit/acq-3540-add-support-for-message-key-in-kp-kafka-producer branch from 20a0400 to 1fa8cfe Compare October 16, 2023 07:34
@rohits1101
rohits1101 force-pushed the rohit/acq-3540-add-support-for-message-key-in-kp-kafka-producer branch from 1fa8cfe to 58a2722 Compare October 16, 2023 07:40
Comment thread v2/producer/producer.go Outdated
}

if key != nil {
msg.Key = key

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If the default value of msg.Key is nil, then can we skip checking if key is nil?

@rohits1101 rohits1101 Oct 17, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Checked, the default is indeed a nil. Pushed a fix to assign the key directly.

Screenshot 2023-10-17 at 2 58 30 PM

kafka:
local-kafka1:
image: confluentinc/cp-kafka:5.5.5
image: confluentinc/cp-kafka:7.3.2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file is supposed to be synced from .github as mentioned in the comment above. Can we follow the same?

tar -xzf kafka_2.13-3.2.3.tgz
./kafka_2.13-3.2.3/bin/kafka-topics.sh --bootstrap-server localhost:9092 --list
./kafka_2.13-3.2.3/bin/kafka-topics.sh --bootstrap-server localhost:9092 --list
wget https://downloads.apache.org/kafka/3.4.1/kafka_2.13-3.4.1.tgz

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure, why this file is not synced from .github though

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Looks like we are performing these extra steps specific to KP. To sync them from workflows, we'd have to copy over these to that repo, and sync from there. Doing that for a single repository doesn't seem worth the effort.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In general i feel this file can very much be synced from .github and maybe use one of the existing ones. If we feel like we need separate steps for integration tests then we can use a build file without the int tests from .github and a custom integration_tests.yaml file.

@rohits1101 rohits1101 Oct 17, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@cyberhck cyberhck left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

keys aren't supported in typed producer by design, they're supposed to be done through middlewares (they're work in progress in #82 ) please make that ready instead.

@rohits1101 rohits1101 closed this Oct 27, 2023
@cyberhck
cyberhck deleted the rohit/acq-3540-add-support-for-message-key-in-kp-kafka-producer branch June 12, 2025 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants