Skip to content

[KIP-932]: Use separate handles for adminclient apis#5388

Open
Pratyush Ranjan (PratRanj07) wants to merge 10 commits intodev_kip-932_queues-for-kafkafrom
dev_kip-932_tests_fixing
Open

[KIP-932]: Use separate handles for adminclient apis#5388
Pratyush Ranjan (PratRanj07) wants to merge 10 commits intodev_kip-932_queues-for-kafkafrom
dev_kip-932_tests_fixing

Conversation

@PratRanj07
Copy link
Copy Markdown
Contributor

No description provided.

@PratRanj07 Pratyush Ranjan (PratRanj07) requested a review from a team as a code owner April 6, 2026 11:04
@confluent-cla-assistant
Copy link
Copy Markdown

🎉 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
Copy Markdown
Member

@pranavrth Pranav Rathi (pranavrth) left a comment

Choose a reason for hiding this comment

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

First pass comments.

always:
commands:
# Upload test 0172 debug log as artifact if it exists (test failed)
- '[ -f artifacts/test_0172_debug.log ] && artifact push job artifacts/test_0172_debug.log --destination debug-logs/test_0172_debug_amd64.log || true'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We might need different file name for different runs as newer file will override the new one and we don't have a way to know which job created the file.

always:
commands:
# Upload test 0172 debug log as artifact if it exists (test failed)
- '[ -f artifacts/test_0172_debug.log ] && artifact push job artifacts/test_0172_debug.log --destination debug-logs/test_0172_debug_amd64.log || true'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update retention to maybe 2 days max.

* @brief Configure share group using a dedicated producer handle.
* Admin client APIs should not reuse the share consumer handle.
*/
static void configure_share_group(const char *group_name,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a generic function which can be moved to the common test file. Can be used even for normal consumer.

  • Use better generic naming.
  • Move to common test.h and test.c files
  • make it extensible for other config types as well

* @brief Delete topic using a dedicated producer handle.
* Admin client APIs should not reuse the share consumer handle.
*/
static void delete_topic_admin(const char *topic) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is a delete topic function already in test common files. Use that internally. What we want to do is to create a function which deletes topics and doesn't take any parameter. Do this in test commons file as well.

* @brief Configure share group using a dedicated producer handle.
* Admin client APIs should not reuse the share consumer handle.
*/
static void configure_share_group(const char *group_name,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use common function.

* @brief Delete topic using a dedicated producer handle.
* Admin client APIs should not reuse the share consumer handle.
*/
static void delete_topic_admin(const char *topic) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Same.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove usage from here as well.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's remove usage of test_share_consumer_get_rk from the tests altogether. Check if we can remove from 0155 as well. Otherwise remove the usage from test.c as well.

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.

In 0155 the test_share_consumer_get_rk is generally used to find the assignments for the particular client instance. I dont think we can remove that usage yet until we modify the underlying functions.

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.

In test.c also it is only used to get the client instance name for the subscribe topics and subscriptions for the logging purpose only. I have changed its usage in the adminclient apis

agent:
machine:
type: s1-macos-15-arm64-8
epilogue:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This need to be enabled in any of the job. I have seen most of the failures in "Linux Ubuntu amd64: integration tests". Check how it can be done.

--version "$TEST_KAFKA_GIT_REF" \
--cpversion "$TEST_CP_VERSION" \
--cmd "TESTS_SKIP_BEFORE=0170 python run-test-batches.py $TEST_ARGS")
--cmd "TESTS_SKIP_BEFORE=0170 TESTS=0170- python run-test-batches.py $TEST_ARGS" 2>&1 | tee "$DEBUG_LOG")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why TESTS=0170-?

@pranavrth
Copy link
Copy Markdown
Member

Update the PR title as its already in review. There is a typo in the title. Fix that as well.

@PratRanj07 Pratyush Ranjan (PratRanj07) changed the title [WIP]: [KIP-932}: Use seperate handles for adminclient apis [KIP-932}: Use separate handles for adminclient apis Apr 8, 2026
@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_kip-932_tests_fixing branch from 0d30b3c to 21d15ab Compare April 8, 2026 17:27
@PratRanj07 Pratyush Ranjan (PratRanj07) changed the base branch from dev_kip-932_queues-for-kafka to dev_kip-932_transaction_and_ctrl_msgs_tests April 8, 2026 17:27
@PratRanj07 Pratyush Ranjan (PratRanj07) changed the title [KIP-932}: Use separate handles for adminclient apis [KIP-932]: Use separate handles for adminclient apis Apr 8, 2026
Base automatically changed from dev_kip-932_transaction_and_ctrl_msgs_tests to dev_kip-932_queues-for-kafka April 9, 2026 06:18
@airlock-confluentinc airlock-confluentinc bot force-pushed the dev_kip-932_tests_fixing branch from 21d15ab to 2721688 Compare April 9, 2026 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants