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

Add delete records admin client api #531

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

imor
Copy link

@imor imor commented Jan 6, 2023

This PR adds bindings to the delete records c API.

Copy link
Collaborator

@benesch benesch left a comment

Choose a reason for hiding this comment

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

Thanks! Code looks about right, but needs a test case to be mergeable.

@imor
Copy link
Author

imor commented Jan 7, 2023

Sure, I'll add a test.

@imor
Copy link
Author

imor commented Jan 8, 2023

@benesch Added tests for the delete records api.

@imor imor requested a review from benesch January 11, 2023 14:16
@imor
Copy link
Author

imor commented Jan 13, 2023

@benesch Can you please take a look at the PR. Thanks.

let res = admin_client.delete_records(&offsets, &opts).await;
assert_eq!(res, Err(KafkaError::AdminOp(RDKafkaErrorCode::NoEnt)));
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is too much code to duplicate to test this feature. Could you instead work the delete_records bit into test_topics?

Copy link
Author

Choose a reason for hiding this comment

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

@benesch Updated tests.

Copy link
Author

Choose a reason for hiding this comment

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

@benesch any update on the review?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @benesch is there anything I can do to help merge this?

@imor imor requested a review from benesch February 7, 2023 10:56
@glebpom
Copy link

glebpom commented Apr 19, 2023

Hi! Any plans to merge it?

@Stranger6667
Copy link

Hey @davidblewett! Just wanted to give you a heads-up that there were some updates to address @benesch's initial feedback on the PR. Totally understand that life gets busy and everyone has their own commitments. If you find a moment to review and potentially merge it, it would be a fantastic help. Thanks a ton!

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.

None yet

4 participants