Skip to content

Conversation

PratRanj07
Copy link
Contributor

This PR aims to:

  • Enabled Describe Consumer Group test for Consumer Group Protocol
  • Test compatibility of describing new protocol consumer group along with old protocol consumer group

@Copilot Copilot AI review requested due to automatic review settings June 25, 2025 14:02
@PratRanj07 PratRanj07 requested a review from a team as a code owner June 25, 2025 14:02
@confluent-cla-assistant
Copy link

🎉 All Contributor License Agreements have been signed. Ready to merge.
Please push an empty commit if you would like to re-run the checks to verify CLA status for all contributors.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enables testing for the Describe Consumer Group API with both the new and legacy consumer group protocols. Key changes include:

  • Adding new functions (test_create_consumer0, test_create_handle0, and test_conf_set0) to support a protocol flag.
  • Updating conditional logic in consumer creation and configuration functions to handle the environment_group_protocol flag.
  • Adjusting timeouts, protocol expectations, and test infrastructure (semaphore configuration) to align with the new protocol.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
tests/test.h Introduces new function test_create_consumer0 with an added environment_group_protocol parameter
tests/test.c Implements test_create_handle0 and updates test_create_consumer logic to conditionally handle protocol changes
tests/0081-admin.c Updates protocol count, session timeout values, and assertions for consumer group tests
.semaphore/semaphore.yml Modifies test cluster invocation command and configuration to match new protocol settings
Comments suppressed due to low confidence (2)

tests/test.h:549

  • [nitpick] Consider using a more descriptive suffix or naming convention for alternative consumer creation functions instead of simply appending '0'.
rd_kafka_t *test_create_consumer0(

tests/test.c:2182

  • [nitpick] Consider renaming 'test_create_handle0' to clarify that this function underlies the public 'test_create_handle' wrapper and accepts an environment_group_protocol flag.
                                rd_bool_t environment_group_protocol) {

Comment on lines 2732 to +2736
/* Create kafka instance */
rk = test_create_handle(RD_KAFKA_CONSUMER, conf);
if (!environment_group_protocol) {
rk = test_create_handle0(RD_KAFKA_CONSUMER, conf,
environment_group_protocol);
} else {
Copy link
Preview

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

[nitpick] The conditional logic in test_create_consumer based on the environment_group_protocol flag may be confusing. Consider adding inline documentation to explain the rationale for the inverted branch behavior.

Suggested change
/* Create kafka instance */
rk = test_create_handle(RD_KAFKA_CONSUMER, conf);
if (!environment_group_protocol) {
rk = test_create_handle0(RD_KAFKA_CONSUMER, conf,
environment_group_protocol);
} else {
/* Create kafka instance */
/* If environment_group_protocol is false, use test_create_handle0.
* This is typically used for scenarios where additional configuration
* or handling is required during consumer creation. */
if (!environment_group_protocol) {
rk = test_create_handle0(RD_KAFKA_CONSUMER, conf,
environment_group_protocol);
} else {
/* If environment_group_protocol is true, use test_create_handle.
* This is the standard consumer creation method. */

Copilot uses AI. Check for mistakes.

test_conf_set(conf, "session.timeout.ms", "5000");
if (!strcmp(protocols[i], "Classic")) {
test_conf_set0(conf, "session.timeout.ms",
"6000", rd_false);
Copy link
Preview

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

[nitpick] The hard-coded value '6000' for the session timeout in the Classic protocol branch is a magic number. Consider introducing a named constant to enhance clarity and maintainability.

Suggested change
"6000", rd_false);
CLASSIC_PROTOCOL_SESSION_TIMEOUT_MS, rd_false);

Copilot uses AI. Check for mistakes.

/* Wait session timeout + 1s. Because using static group membership */
rd_sleep(6);
/* Wait session timeout + 2s. Because using static group membership */
rd_sleep(8);
Copy link
Preview

Copilot AI Jun 25, 2025

Choose a reason for hiding this comment

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

[nitpick] The increased sleep duration (from 6 to 8 seconds) is hard-coded. Consider defining this value as a constant to document its intent and simplify future adjustments.

Suggested change
rd_sleep(8);
rd_sleep(SESSION_TIMEOUT_SLEEP_DURATION);

Copilot uses AI. Check for mistakes.

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.

1 participant