Skip to content

custreamz oauth callback for kafka (librdkafka) - #9486

Merged
rapids-bot[bot] merged 84 commits into
NVIDIA:branch-22.02from
jdye64:custreamz_oauth
Jan 6, 2022
Merged

custreamz oauth callback for kafka (librdkafka)#9486
rapids-bot[bot] merged 84 commits into
NVIDIA:branch-22.02from
jdye64:custreamz_oauth

Conversation

@jdye64

@jdye64 jdye64 commented Oct 21, 2021

Copy link
Copy Markdown
Contributor

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

@jdye64
jdye64 requested review from a team as code owners October 21, 2021 14:00
@github-actions github-actions Bot added CMake CMake build issue Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Oct 21, 2021
@jdye64
jdye64 marked this pull request as draft October 21, 2021 14:01
@jdye64
jdye64 marked this pull request as ready for review October 25, 2021 18:17

@robertmaynard robertmaynard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

CMake changes 👍

@github-actions github-actions Bot added the conda label Oct 26, 2021
@jdye64
jdye64 requested a review from a team as a code owner October 26, 2021 20:15
@vuule vuule added non-breaking Non-breaking change feature request New feature or request labels Oct 26, 2021

@ajschmidt8 ajschmidt8 left a comment

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'll also need to update the librdkafka version in the integration repo below. @jdye64, can you open a PR for that?

@jdye64

jdye64 commented Oct 27, 2021

Copy link
Copy Markdown
Contributor Author

We'll also need to update the librdkafka version in the integration repo below. @jdye64, can you open a PR for that?

Done - rapidsai/integration#386

@jdye64

jdye64 commented Dec 14, 2021

Copy link
Copy Markdown
Contributor Author

rerun tests

3 similar comments
@ajschmidt8

Copy link
Copy Markdown
Member

rerun tests

@jdye64

jdye64 commented Dec 15, 2021

Copy link
Copy Markdown
Contributor Author

rerun tests

@jdye64

jdye64 commented Dec 17, 2021

Copy link
Copy Markdown
Contributor Author

rerun tests

Comment thread conda/recipes/custreamz/meta.yaml Outdated

@vyasr vyasr left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread ci/gpu/build.sh
Comment thread cpp/libcudf_kafka/include/cudf_kafka/kafka_consumer.hpp
Comment thread cpp/libcudf_kafka/include/cudf_kafka/kafka_consumer.hpp
Comment thread cpp/libcudf_kafka/include/cudf_kafka/kafka_callback.hpp
Comment thread cpp/libcudf_kafka/src/kafka_callback.cpp Outdated
Comment thread python/cudf_kafka/cudf_kafka/_lib/kafka.pxd Outdated
Comment thread python/cudf_kafka/cudf_kafka/_lib/kafka.pyx Outdated
Comment thread python/cudf_kafka/cudf_kafka/_lib/kafka.pyx
Comment thread python/custreamz/custreamz/kafka.py
Comment thread python/custreamz/custreamz/tests/test_kafka.py
jdye64 and others added 2 commits December 22, 2021 14:39
Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>
Co-authored-by: Vyas Ramasubramani <vyas.ramasubramani@gmail.com>
@raydouglass
raydouglass removed the request for review from a team January 5, 2022 15:35

@ramakrishnap-nv ramakrishnap-nv left a comment

Copy link
Copy Markdown
Contributor

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

Comment thread cpp/libcudf_kafka/include/cudf_kafka/kafka_callback.hpp Outdated
@ajschmidt8
ajschmidt8 dismissed harrism’s stale review January 6, 2022 17:19

Seems that all requested changes have been addressed.

@ajschmidt8

Copy link
Copy Markdown
Member

@gpucibot merge

@rapids-bot
rapids-bot Bot merged commit 23603d1 into NVIDIA:branch-22.02 Jan 6, 2022
@bdice

bdice commented Jan 12, 2022

Copy link
Copy Markdown
Contributor

@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

@vyasr

vyasr commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

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 *.swp with a broader gitignore rule like *.sw[a-p] would definitely be helpful to avoid this kind of issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] custreamz fails to connect to kafka with oauth login