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 'poll' function to custreamz kafka consumer #13782

Merged
merged 7 commits into from
Aug 14, 2023

Conversation

jdye64
Copy link
Contributor

@jdye64 jdye64 commented Jul 28, 2023

Description

Streamz has updated their codebase to include a call to the Confluent Kafka Consumer library function 'poll'. Currently custreamz does not include this method. This PR adds the 'poll' function to custreamz to simply proxy the call to the underlying confluent kafka library so that streamz is no longer broken for end users. Without this function end users are no longer able to use custreamz with newer versions of the streamz library.

This closes: #13600

@jdye64 jdye64 requested a review from a team as a code owner July 28, 2023 14:41
@jdye64 jdye64 requested review from vyasr and isVoid July 28, 2023 14:41
@github-actions github-actions bot added the Python Affects Python cuDF API. label Jul 28, 2023
@bdice
Copy link
Contributor

bdice commented Jul 28, 2023

Does this need to target 23.08 or 23.10? Breakage with new versions of streamz sounds like a candidate for 23.08 despite code freeze.

@jdye64
Copy link
Contributor Author

jdye64 commented Jul 28, 2023 via email

@jdye64
Copy link
Contributor Author

jdye64 commented Jul 28, 2023

@bdice I don't have the permissions to alter the labels, hence Label Checker CI failure. Can you add those for me?

@vyasr vyasr added feature request New feature or request 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Jul 28, 2023
@vyasr
Copy link
Contributor

vyasr commented Jul 28, 2023

Added labels. I assumed that this could be treated as a new feature even though technically we would be broken by the new version of the library.

@chinmaychandak
Copy link
Contributor

Hey guys, would it be possible to merge this PR some time this week? This is blocking our cuStreamz dev work.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

@jdye64 Are there dependency changes needed to support this? Are these pinnings still accurate or do they need to be updated?

cudf/dependencies.yaml

Lines 496 to 508 in 989c411

run_custreamz:
common:
- output_types: conda
packages:
- python-confluent-kafka>=1.9.0,<1.10.0a0
- output_types: [conda, requirements, pyproject]
packages:
- streamz
- output_types: [requirements, pyproject]
packages:
- confluent-kafka>=1.9.0,<1.10.0a0
- *cudf
- cudf_kafka==23.10.*

- python-confluent-kafka >=1.9.0,<1.10.0a0

python/custreamz/custreamz/kafka.py Outdated Show resolved Hide resolved
@jdye64
Copy link
Contributor Author

jdye64 commented Aug 14, 2023

@bdice the

@jdye64 Are there dependency changes needed to support this? Are these pinnings still accurate or do they need to be updated?

cudf/dependencies.yaml

Lines 496 to 508 in 989c411

run_custreamz:
common:
- output_types: conda
packages:
- python-confluent-kafka>=1.9.0,<1.10.0a0
- output_types: [conda, requirements, pyproject]
packages:
- streamz
- output_types: [requirements, pyproject]
packages:
- confluent-kafka>=1.9.0,<1.10.0a0
- *cudf
- cudf_kafka==23.10.*

- python-confluent-kafka >=1.9.0,<1.10.0a0

No, the dependencies are still fine as is. We didn't add a new function but rather just added a hook to call one that had always existed.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Looks fine to me. @jdye64 Do we need to add tests for this?

Do you have merge privileges? Do you want me to merge after CI passes?

@jdye64
Copy link
Contributor Author

jdye64 commented Aug 14, 2023

@bdice tests would be nice but we decided awhile back it wasn't worth the significant CI burden of creating a kafka cluster during each PR, took almost 45 minutes to install, setup, test, and breakdown when we did it back a year or so ago. Ultimately deciding that this library is well tested upstream and we were not gaining anything by testing it again.

I do not have merge permissions. Would be great if you could merge once CI passes. Thanks!

@bdice
Copy link
Contributor

bdice commented Aug 14, 2023

/merge

@rapids-bot rapids-bot bot merged commit 5a92416 into rapidsai:branch-23.10 Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Add a poll method to cuStreamz for single messages
4 participants