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 send_batch method to kafka mod #324

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

cai-n
Copy link
Contributor

@cai-n cai-n commented Dec 18, 2024

Add a new method to the kafka mod to also send with rdkafka's internal batching. The return is a bit strange because I couldn't figure out how to send the failed indexes as LuaError that could then be used in the lua part if it was caught with a pcall...

Copy link
Collaborator

@wez wez left a comment

Choose a reason for hiding this comment

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

I was just wondering how you were getting on with this the other day!

crates/mod-kafka/Cargo.toml Outdated Show resolved Hide resolved
crates/mod-kafka/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@wez wez left a comment

Choose a reason for hiding this comment

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

Looks good!
Please also add docs for this to docs/reference/kumo.kafka/build_producer.md

You can use {{ since('dev') }} in the section that you add there to indicate that this is available in dev.

Thanks!

rdkafka.workspace=true
serde.workspace=true
tokio.workspace=true
tracing.workspace=true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can trim this out with the API change to return the errors instead of logging them

Suggested change
tracing.workspace=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I'll remove it

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