Skip to content

KAFKA-2846: Add Ducktape test for kafka-consumer-groups#555

Closed
SinghAsDev wants to merge 3 commits intoapache:trunkfrom
SinghAsDev:KAFKA-2846
Closed

KAFKA-2846: Add Ducktape test for kafka-consumer-groups#555
SinghAsDev wants to merge 3 commits intoapache:trunkfrom
SinghAsDev:KAFKA-2846

Conversation

@SinghAsDev
Copy link
Copy Markdown
Contributor

No description provided.

@SinghAsDev
Copy link
Copy Markdown
Contributor Author

@granders @guozhangwang mind taking a look?

@SinghAsDev
Copy link
Copy Markdown
Contributor Author

@granders @guozhangwang pinging again for review. I just rebased it over latest trunk. Thanks!

@SinghAsDev
Copy link
Copy Markdown
Contributor Author

@ewencp do you mind taking a look at this, it has been pending for quite some time now. Thanks!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How necessary is it to test this with SSL? @hachikuji any thoughts on this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why all the different security variants? Are we actually concerned that these wouldn't work -- are the different variants actually providing value? If they are, is there any reason to have the variants on all of these tests rather than just on 1 of them (i.e. why would we think that if it works using a secured cluster on test_list_consumer_groups, why wouldn't it on test_describe_consumer_groups)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@granders I agree with @ewnecp that covering all the protocols doesn't seem to add much value.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question. Below are the combinations for ConsumerGroupCommand.
OldConsumer
NewConsumer
NewConsumer + Security

I chose to test following combinations and I think it is reasonable.
OldConsumer
NewConsumer + Security

I also think these tests are to make sure we catch regressions in future. If something is working today, it does not mean it will always work. Also, note that I am not testing against all protocols.

why would we think that if it works using a secured cluster on test_list_consumer_groups, why wouldn't it on test_describe_consumer_groups

Sure, it works today. These tests are supposed to check regressions. {{LIST_GROUPS}} and {{DESCRIBE_GROUPS}} have different code paths, request/responses, etc. I see that as a strong reason to test both of them.

@SinghAsDev
Copy link
Copy Markdown
Contributor Author

Above test errors are not related. @ewencp @granders mind talking a look again? Thanks!

@asfgit asfgit closed this in 4f39b5b Jan 24, 2016
C0urante pushed a commit to C0urante/kafka that referenced this pull request May 26, 2021
juha-aiven pushed a commit to aiven/kafka that referenced this pull request Apr 2, 2026
…on for classic-to-diskless migration (apache#555)

When diskless.allow.from.classic.enable is true and a topic is being migrated
from classic (remote.storage.enable=true) to diskless, skip the mutual exclusion
validation that prevents both configs from being set simultaneously.

The bypass only triggers when all conditions are met:
- diskless.allow.from.classic.enable is true (broker config)
- diskless.enable is being explicitly set to true (request)
- remote.storage.enable was previously set to true (existing topic config)
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.

4 participants