Skip to content

Conversation

@tdas
Copy link
Contributor

@tdas tdas commented Jul 26, 2016

What changes were proposed in this pull request?

The current test is incorrect, because

  • The expected number of messages does not take into account that the topic has 2 partitions, and rate is set per partition.
  • Also in some cases, the test ran out of data in Kafka while waiting for the right amount of data per batch.

The PR

  • Reduces the number of partitions to 1
  • Adds more data to Kafka
  • Runs with 0.5 second so that batches are created slowly

How was this patch tested?

Ran many times locally, going to run it many times in Jenkins

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

@tdas
Copy link
Contributor Author

tdas commented Jul 26, 2016

@koeninger Can you take a look?

@SparkQA
Copy link

SparkQA commented Jul 26, 2016

Test build #62861 has finished for PR 14361 at commit 52b5a20.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 26, 2016

Test build #3192 has finished for PR 14361 at commit 52b5a20.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 26, 2016

Test build #3191 has finished for PR 14361 at commit 52b5a20.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@tdas
Copy link
Contributor Author

tdas commented Jul 26, 2016

I also ran it on my machine over 250 times, not a single failure. So I am merging this to master and 2.0.

asfgit pushed a commit that referenced this pull request Jul 26, 2016
## What changes were proposed in this pull request?

The current test is incorrect, because
- The expected number of messages does not take into account that the topic has 2 partitions, and rate is set per partition.
- Also in some cases, the test ran out of data in Kafka while waiting for the right amount of data per batch.

The PR
- Reduces the number of partitions to 1
- Adds more data to Kafka
- Runs with 0.5 second so that batches are created slowly

## How was this patch tested?
Ran many times locally, going to run it many times in Jenkins

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Author: Tathagata Das <tathagata.das1565@gmail.com>

Closes #14361 from tdas/kafka-rate-test-fix.

(cherry picked from commit 03c2743)
Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
@asfgit asfgit closed this in 03c2743 Jul 26, 2016
@koeninger
Copy link
Contributor

  • This is testing RateEstimator, not maxRatePerPartition. I didn't write the rate estimator code, but my understanding of the rate expressed there is that it is on a per-stream basis, not a per-partition basis. So your explanation of why partitions need to be reduced to 1 doesn't make sense to me.
  • Even if that is the case, it seems like a better idea to fix the expected sizes, not limit to 1 partition, because people will be using backpressure with multi-partition topics
  • This test exists in both 0.8 and 0.10, but this patch only applies to 0.10

Just as kind of a meta-comment, I'm not sure what the point of asking for feedback is if it's going to be merged to master within 5 hours regardless. I was asleep during that entire time. I understand the rush for 2.0, and I'm not trying to play the "Apache Process" card or get in your face... I'd just ask you to consider the reasoning involved.

@tdas
Copy link
Contributor Author

tdas commented Jul 26, 2016

Flaky test fixes are always a higher priority for merging because it blocks productivity for others. Often we have ignored tests at very short notice to unblock others. Nonetheless, my apologies for not explicitly mentioning it, but I was welcoming your feedback nonetheless as we can always incorporate your feedback in another PR.

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.

3 participants