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

Merge two tests that are very similar #1891

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeffwidman
Copy link
Collaborator

@jeffwidman jeffwidman commented Aug 23, 2019

Previously the test_kafka_consumer_max_bytes_simple() was seeing
occasional test failures because it was doing only 10 iterations. And
much of the purpose of it was gutted when Kafka 0.11 came out and
changed the behavior. So this merges the two tests into one which should
be relatively straightforward.

Further discussion in https://github.com/dpkp/kafka-python/pull/1886/files#r316860737


This change is Reviewable

@tvoinarovskyi
Copy link
Collaborator

Hmm, seems to me like the test actually fails for a valid reason here. We don't have round robin partition rotation in fetch requests like Java does. We will probably have starvation. It's strange thou, as I clearly remember we added a random shuffle to partition order, maybe I've mistaken something.

@tvoinarovskyi
Copy link
Collaborator

I may look into it later if you don't want to waste time on it. Just mark expected fail for now.

@jeffwidman jeffwidman force-pushed the cleanup-max-bytes-tests branch from f44516e to 410f03b Compare August 23, 2019 07:05
Previously the `test_kafka_consumer_max_bytes_simple()` was seeing
occasional test failures because it was doing only 10 iterations. And
much of the purpose of it was gutted when Kafka 0.11 came out and
changed the behavior. So this merges the two tests into one which should
be relatively straightforward.

Further discussion in https://github.com/dpkp/kafka-python/pull/1886/files#r316860737
@jeffwidman jeffwidman force-pushed the cleanup-max-bytes-tests branch from 410f03b to e5683ed Compare August 23, 2019 07:23
@jeffwidman
Copy link
Collaborator Author

K, for now I kept at 10 iterations, just like originally. And I also remember seeing the comment in the code about random shuffle for partition. That probably explains why sometimes it succeeds, and other times it fails... it seems to be somewhat non-deterministic, although I haven't dug into it deeply.

Regardless, I did keep them merged, as I think the resulting single test easily covers both cases. The only thing we lost by merging was one of the tests had used the poll() interface and the other used the iterator (next()) interface. I don't think it's worrying about though, as the underlying bit this is trying to test AFAIK should be the same with both code paths.

I'll see what happens on the tests, and if necessary can mark it as xfail

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