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

Allow purging (Purge API) / don't leak on dropping producer #520

Merged
merged 8 commits into from
Apr 17, 2023

Conversation

Ten0
Copy link
Contributor

@Ten0 Ten0 commented Dec 4, 2022

Resolves #518

What it does:

  • Purges producer on drop
  • Updates bindgen script to more recent bindgen version
  • Sets the parameter that makes macro constants signed (because that's how they are generally used in librdkafka)

Todo:

  • Check that it works
  • Expose the full interface

Resolves fede1024#518

**What it does:**
- Purges producer on drop
- Updates bindgen script to more recent bindgen version
- Sets the parameter that makes macro constants signed (because that's how they are generally used in librdkafka)
@Ten0
Copy link
Contributor Author

Ten0 commented Dec 5, 2022

Could use a ci run as well 🙂

@Ten0 Ten0 marked this pull request as ready for review December 10, 2022 12:44
@Ten0
Copy link
Contributor Author

Ten0 commented Dec 10, 2022

This is ready.

I went ahead and pulled the trigger on removing the Clone bound on the BaseProducer in ea0e685 because it appears to really be unnecessary and overcomplexifies stuff for no gain.

It seems similarly unnecessary on the FutureProducer at the moment, but because there's runtime awareness with spawn capabilities I could actually imagine that it may be useful to be able to internally spawn tasks that refer to it some day, and it doesn't really complexify anything there (it doesn't need a particular Drop impl) so it seems reasonable to leave internal reference counting there for now.

@Ten0
Copy link
Contributor Author

Ten0 commented Jan 7, 2023

@benesch

If you've got the time, please submit a PR! The maintainers don't have any spare bandwidth these days, I'm afraid.

Since you suggested that I dedicate time to implementing this... when could I hope for this to be reviewed?

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.

I'm afraid I only have an hour or two every few months to spend on rust-rdkafka these days.

This looks pretty solid. Thanks again for sending it up. Just one comment about the use of bitflags.

--raw-line "use num_enum::TryFromPrimitive;" \
--default-macro-constant-type "signed" \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this important for this PR? Or just avoiding an as i32 somewhere?

Copy link
Contributor Author

@Ten0 Ten0 Jan 14, 2023

Choose a reason for hiding this comment

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

It was avoiding as i32 in my code as well as in existing code. Overall this seems to be how macro constants are used in librdkafka, so that seems to be the right way to represent these.

/// Don't wait for background thread queue purging to finish.
const NON_BLOCKING = rdkafka_sys::RD_KAFKA_PURGE_F_NON_BLOCKING;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like it would be Rust-ier to use a PurgeConfig struct here with queue(bool), in_flight(bool), and non_blocking(bool) methods. No particular need to save the two bytes. Also avoids the dependency on the bitflags crate, which idk someone will probably care about. (We get a lot of PRs to minimize crate dependencies.)

Copy link
Contributor Author

@Ten0 Ten0 Jan 14, 2023

Choose a reason for hiding this comment

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

So that is something I initially considered and I changed my mind twice about it.
And here is one more ^^

Done in 37e3180

@davidblewett
Copy link
Collaborator

@Ten0 we're preparing a release of v0.30.0. Can you update this PR to fix the merge conflicts so we can include it?

@Ten0
Copy link
Contributor Author

Ten0 commented Feb 28, 2023

done

@davidblewett davidblewett merged commit 611aeea into fede1024:master Apr 17, 2023
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.

Allow purging (purge API) / don't leak on dropping producer
4 participants