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

Update kafka version to include client side gzip leak fix #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

mridulm
Copy link

@mridulm mridulm commented Apr 6, 2018

This is the same pr which I submitted against @jerryshao's older repo.
I have also bumped up the artifact version due to dependency version change.

@mridulm
Copy link
Author

mridulm commented Apr 6, 2018

+CC @bikassaha @jerryshao

@mridulm
Copy link
Author

mridulm commented Apr 7, 2018

Two tests failed again:

- basic stream receiving with multiple topics and smallest starting offset *** FAILED *** (1 minute, 42 seconds)
  The code passed to eventually never returned normally. Attempted 110 times over 1.6734079014833332 minutes. Last failure message: 0 did not equal 46 didn't get expected number of messages, messages:
  . (DirectKafkaStreamSuite.scala:152)
- pattern based subscription *** FAILED *** (1 minute, 41 seconds)
  The code passed to eventually never returned normally. Attempted 110 times over 1.672780918083333 minutes. Last failure message: 0 did not equal 41 didn't get expected number of messages, messages:
  . (DirectKafkaStreamSuite.scala:218)

Is this a kafka incompatibility ? Or flakey tests ?

@jerryshao
Copy link
Contributor

@mridulm
Copy link
Author

mridulm commented Apr 8, 2018

Looks like it is not a straightforward case of upgrading dependency.
Given that skc is a convenience package to transition for older users, I am thinking of closing this PR and avoiding hassle with having to fix/make increasing code changes.
Any thoughts @jerryshao @bikassaha ?

@jerryshao
Copy link
Contributor

If we ignore the UT failure, then I think it is OK to upgrade the Kafka version. This is because test code uses several internal APIs which doesn't guarantee to be compatible across version, whereas source code only depends on kafka-client APIs, which guarantees to be compatible, so it is OK to upgrade the Kafka version if we ignore the UT failures.

@mridulm
Copy link
Author

mridulm commented Apr 9, 2018

@jerryshao Ignoring UT failures, which used to work with earlier versions, is a bit worrying; we potentially miss out of issues (particularly when evolving code).
If you believe it is non trivial to fix UT's (I really dont have much experience with streaming - but I can take a shot if it is something simple), I would rather abandon this change than make codebase unstable.

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