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.

Sign up for GitHub

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

Jump to bottom

fix: Fix conversion of Kafka ConfigEntry object to it's corresponding Vertx Kafka client object #287

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

fix: Fix conversion of Kafka ConfigEntry object to it's corresponding Vertx Kafka client object #287

wants to merge 2 commits into from
+150 −1

Conversation

Kartikey-Mishra1
Copy link

@Kartikey-Mishra1 Kartikey-Mishra1 commented Jan 20, 2025 •
edited
Loading

Motivation:

  • Currently the constructor of the ConfigEntry class creates objects by only initializing the name and value fields. This means that when an object of this class is created to represent a Kafka ConfigEntry object, the properties such as ConfigSource, isSensitive, isReadOnly and ConfigSynonym of the Kafka object are never mapped to the new object.

Description:

  • This PR adds a new constructor to the ConfigEntry class which initializes these additional fields along with name and value fields. The new constructor shares a lot of similarities with this constructor in order to keep this class aligned with its Kafka counterpart.
  • The new constructor is hence used instead of the old one to convert Kafka ConfigEntry objects to objects of the above ConfigEntry class.

Closes #285

tsegismont
tsegismont reviewed Jan 21, 2025
View reviewed changes
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

@Kartikey-Mishra1 thanks for this PR. Overall, it looks good to me. Can you make sure your IDE has a plugin that reads the editor config file and uses it to format the code properly? Thank you

@tsegismont tsegismont requested a review from ppatierno January 21, 2025 09:13
@ppatierno
Copy link
Member

LGTM. Just wondering if you should double check the tests failures even if they don't seem related to the PR.

@Kartikey-Mishra1
style: format using .editorconfig
a64c8bf 
Signed-off-by: Kartikey-Mishra1 <[email protected]>
@Kartikey-Mishra1
Copy link
Author

Hi @ppatierno the failing test (ProducerMockTest.testWriteWithSimulatedError) seems to fail for me locally on my branch and the master branch. I see that one of the scheduled test runs on master also failed due to this test failing. So this is probably a test problem and is maybe not related to my changes. Do you agree or need me to investigate further?

@Kartikey-Mishra1
Copy link
Author

Kartikey-Mishra1 commented Jan 22, 2025 •
edited
Loading

Hi @tsegismont @ppatierno can you please advise on how to proceed forward here? I don't think my changes are causing the test failures as 5.x builds from master also seem to be failing https://github.com/vert-x3/vertx-kafka-client/actions/runs/12901560323.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsegismont tsegismont tsegismont left review comments

@ppatierno ppatierno Awaiting requested review from ppatierno

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Source of ConfigEntry is always null
3 participants
@Kartikey-Mishra1 @ppatierno @tsegismont