-
Notifications
You must be signed in to change notification settings - Fork 915
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
custreamz oauth callback for kafka (librdkafka) #9486
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake changes 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also need to update the librdkafka
version in the integration
repo below. @jdye64, can you open a PR for that?
…Adjusted unit tests to account for this
Done - rapidsai/integration#386 |
rerun tests |
1 similar comment
rerun tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new version looks so much better! I'm happy that you were able to avoid the Python C API to this extent. I have a few minor inline comments, but hopefully nothing too painful left to deal with. I also have a few questions that didn't fit inline anywhere:
- There are a lot of build script changes in this PR that seem unrelated to enabling the kafka callbacks, what's the reason for those?
- A
.swo
file got committed, can you remove it? I assume you have.swp
in your.gitignore
but had multiple copies opened at different times (or had a vim crash) so it created a.swo
file. - Can we add a Python test of the callback as well as the C++ test? The only Python test change that I see seems completely unrelated.
Co-authored-by: Vyas Ramasubramani <[email protected]>
Co-authored-by: Vyas Ramasubramani <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, need to update year in files since its 2022. And if possible resolve the following issue https://github.com/rapidsai/cudf/pull/9486/files#r751707416.
Rest looks good
Seems that all requested changes have been addressed.
@gpucibot merge |
@jdye64 It looks like this PR accidentally added a 12KB binary temp file from vim, in commit jdye64@7932468. This was missed during review and is now in the main repo: https://github.com/rapidsai/cudf/blob/76f89db80a64a2aa49b618aad80fe80e34e0332f/python/cudf_kafka/cudf_kafka/_lib/.kafka.pxd.swo Can you please open a PR to remove this file? Thank you! This gitignore may also be helpful: https://github.com/github/gitignore/blob/main/Global/Vim.gitignore |
Hmm I specifically requested to get rid of that in some round of review and I thought I saw it deleted in one of the subsequent PRs. Unfortunate that it crept back in. Replacing |
Previously it was impossible to use custreamz with oauth enabled Kafka brokers. This PR adds a feature so that the user can supply a Python function which is invoked to get the oauth token, from a http endpoint for example, and then supply that token to librdkafka to be used in both the initial connection to kafka and also subsequently as the token becomes stale.
This closes #9410