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

Describe cluster API Support #1250

Merged
merged 7 commits into from
Sep 18, 2024

Conversation

akrambek
Copy link
Contributor

Description

See 1058

Comment on lines 591 to 592
struct ClusterBroker
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
struct ClusterBroker
{
struct ClusterBroker
{

string16 host;
int32 port;
string16 rack = null;
}
Copy link
Contributor

@jfallows jfallows Sep 17, 2024

Choose a reason for hiding this comment

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

Let's move this structure down between DescribeClusterResponse and DescribeClusterResponsePart2 as it is part of the response and logically belongs between these structures.

Comment on lines 560 to 563
case 60: kafka::stream::KafkaDescribeClusterRequestBeginEx describeCluster;
case 33: kafka::stream::KafkaAlterConfigsRequestBeginEx alterConfigs;
case 20: kafka::stream::KafkaDeleteTopicsRequestBeginEx deleteTopics;
case 19: kafka::stream::KafkaCreateTopicsRequestBeginEx createTopics;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's put these in the order of their api key, increasing.

Comment on lines 41 to 42
.brokerId(0)
.host("localhost0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use more meaningful broker ids and host names, like 0xb1 and broker-1.internal.net or similar.
I think we might have some examples of that in other scripts.

@akrambek akrambek marked this pull request as ready for review September 17, 2024 23:49
@jfallows jfallows merged commit 615da8f into aklivity:develop Sep 18, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants