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

Add capability to add topic configuration during topic creation #35824

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Add capability to add topic configuration during topic creation #35824

merged 1 commit into from
Dec 4, 2024
+45 −14

Conversation

f0nZ
Copy link
Contributor

@f0nZ f0nZ commented Sep 8, 2023

Updated the createTopic method in the KafkaAdminClient class to accept an optional configs parameter in the form of a Map<String, String>.
This parameter allows users to specify custom configuration settings for the Kafka topic.

@f0nZ f0nZ marked this pull request as ready for review September 8, 2023 18:24
phillip-kruger
phillip-kruger approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@phillip-kruger phillip-kruger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM from a Dev UI perspective. I would also ask @ozangunalp and/or @cescoffier to have a look from a feature p.o.v

@quarkus-bot

This comment has been minimized.

Sign in to view
cescoffier
cescoffier requested changes Sep 11, 2023
View reviewed changes
...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/KafkaAdminClient.java Show resolved Hide resolved
...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/KafkaAdminClient.java Show resolved Hide resolved
...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/KafkaAdminClient.java
Collection<String> topics = new ArrayList<>();
topics.add(name);
DeleteTopicsResult dtr = client.deleteTopics(topics);
return dtr.topicNameValues() != null;
}

public boolean createTopic(KafkaCreateTopicRequest kafkaCreateTopicRq) {
public boolean createTopic(final KafkaCreateTopicRequest kafkaCreateTopicRq) {
var partitions = Optional.ofNullable(kafkaCreateTopicRq.getPartitions()).orElse(1);
var replications = Optional.ofNullable(kafkaCreateTopicRq.getReplications()).orElse((short) 1);
Copy link
Member

Choose a reason for hiding this comment

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

If you use the dev service, I don't believe you can use more than 1, as there is a single node.
So, we may want to disable that option in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify what you mean by "disabling the option"?
Are you asking if to remove the default value?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, or add a message in the UI.

...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/KafkaAdminClient.java Show resolved Hide resolved
...-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/devui/KafkaJsonRPCService.java Show resolved Hide resolved
...-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/devui/KafkaJsonRPCService.java Show resolved Hide resolved
...-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/devui/KafkaJsonRPCService.java Show resolved Hide resolved
@f0nZ f0nZ force-pushed the main branch 3 times, most recently from fb7d3c2 to 89af2b0 Compare September 11, 2023 08:28
@f0nZ f0nZ requested a review from gsmet September 11, 2023 08:42
ozangunalp
ozangunalp requested changes Sep 11, 2023
View reviewed changes
Copy link
Contributor

@ozangunalp ozangunalp left a comment

Choose a reason for hiding this comment

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

Nice addition, but it needs some work for the UI field for topic config input.

extensions/kafka-client/deployment/src/main/resources/dev-ui/qwc-kafka-add-topic.js
@@ -59,6 +59,14 @@ export class QwcKafkaAddTopic extends LitElement {
min="0"
max="99">
</vaadin-integer-field>
<vaadin-text-field
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that field would work to populate configs as Map<String, String>.

Copy link
Member

Choose a reason for hiding this comment

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

@phillip-kruger ^

...ons/kafka-client/runtime/src/main/java/io/quarkus/kafka/client/runtime/KafkaAdminClient.java Show resolved Hide resolved
@gsmet
Copy link
Member

gsmet commented Jun 7, 2024

@ozangunalp @phillip-kruger @cescoffier it would be great to finalize this PR.

@cescoffier
Copy link
Member

@gsmet I believe it would need to be rewritten.

@cescoffier
Copy link
Member

@ozangunalp What do you think about this one? Should we rework it or close it?

@cescoffier
Copy link
Member

@ozangunalp Can you check my last comment?

@ozangunalp
Copy link
Contributor

I'll give a quick look to that branch. Maybe it is not that far from complete.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@cescoffier
Copy link
Member

@ozangunalp can you squash the commits?

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Dec 3, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 5b0dfd5.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@cescoffier cescoffier merged commit c5cc765 into quarkusio:main Dec 4, 2024
24 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ozangunalp ozangunalp ozangunalp requested changes

@phillip-kruger phillip-kruger phillip-kruger approved these changes

@cescoffier cescoffier cescoffier approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/kafka
Projects
None yet
Milestone
3.18 - main
Development

Successfully merging this pull request may close these issues.
5 participants
@f0nZ @gsmet @cescoffier @ozangunalp @phillip-kruger