Skip to content

MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters.#7982

Merged
ijuma merged 3 commits intoapache:trunkfrom
bdbyrne:producer-close-test
Apr 28, 2020
Merged

MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters.#7982
ijuma merged 3 commits intoapache:trunkfrom
bdbyrne:producer-close-test

Conversation

@bdbyrne
Copy link
Copy Markdown
Contributor

@bdbyrne bdbyrne commented Jan 18, 2020

Makes the main thread wait for workers to be ready to test the
desired functionality before proceeding.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@bdbyrne bdbyrne requested a review from hachikuji January 18, 2020 17:26
@bdbyrne bdbyrne mentioned this pull request Jan 18, 2020
Merged
3 tasks
tedyu
tedyu reviewed Jan 18, 2020
View reviewed changes
Comment thread clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java Outdated
Copy link
Copy Markdown
Contributor

@tedyu tedyu Jan 18, 2020

Choose a reason for hiding this comment

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

Do you know why the sleep is needed ?

From my experiment, 1 millis sleep suffices.

The timeout for completed.await() below can be shortened as well since the allocate() call would return immediately.

Copy link
Copy Markdown
Contributor Author

@bdbyrne bdbyrne Jan 18, 2020

Choose a reason for hiding this comment

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

It's not necessary, but was done to ensure better predictability.

In the latch implementation, it may notify a futex which, depending on the scheduler, may pause the worker thread and schedule back to the main thread which would call close() before the worker was waiting in the buffer pool. Adding a sleep deschedules the main thread again so that the worker may continue.

1 ms should accomplish the same thing. As @hachikuji noted in the previous PR, timeouts should be fairly lax so that they don't induce flakiness when running on Jenkins, so I'll defer to him on any modifications there.

tedyu
tedyu reviewed Jan 18, 2020
View reviewed changes
Comment thread clients/src/test/java/org/apache/kafka/clients/producer/internals/BufferPoolTest.java Outdated
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Mar 24, 2020

retest this please

@ijuma ijuma requested a review from rajinisivaram March 24, 2020 14:54
Brian added 3 commits April 27, 2020 12:34
MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters.
d657a6f 
Makes the main thread wait for workers to be ready to test the
desired functionality before proceeding.
Reduce sleep timeout.
cc46cfd
Address review comment.
72b1fd9
@bdbyrne bdbyrne force-pushed the producer-close-test branch from 1f9632d to 72b1fd9 Compare April 27, 2020 20:03
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Apr 27, 2020

retest this please

@bdbyrne
Copy link
Copy Markdown
Contributor Author

bdbyrne commented Apr 28, 2020 •
edited
Loading

Test failure unrelated:

kafka.api.CustomQuotaCallbackTest > testCustomQuotaCallback STARTED
kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup failed, log available in /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk14-scala2.13/core/build/reports/testOutput/kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup.test.stdout

kafka.api.ConsumerBounceTest > testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup FAILED
    org.scalatest.exceptions.TestFailedException: The remaining consumers in the group could not fetch the expected records
        at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:530)
        at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:529)
        at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1389)
        at org.scalatest.Assertions.fail(Assertions.scala:1091)
        at org.scalatest.Assertions.fail$(Assertions.scala:1087)
        at org.scalatest.Assertions$.fail(Assertions.scala:1389)
        at kafka.api.ConsumerBounceTest.testRollingBrokerRestartsWithSmallerMaxGroupSizeConfigDisruptsBigGroup(ConsumerBounceTest.scala:331)

@ijuma ijuma merged commit 2eb0ccf into apache:trunk Apr 28, 2020
@ijuma
Copy link
Copy Markdown
Member

ijuma commented Apr 28, 2020

LGTM, thanks.

ijuma added a commit to ijuma/kafka that referenced this pull request Apr 29, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
ebf63f7 
…t-for-generated-requests

* apache-github/trunk: (366 commits)
  MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982)
  MINOR: document how to escape json parameters to ducktape tests (apache#8546)
  KAFKA-9885; Evict last members of a group when the maximum allowed is reached (apache#8525)
  KAFKA-9866: Avoid election for topics where preferred leader is not in ISR (apache#8524)
  KAFKA-9839; Broker should accept control requests with newer broker epoch (apache#8509)
  KAKFA-9612: Add an option to kafka-configs.sh to add configs from a prop file (KIP-574)
  MINOR: Partition is under reassignment when adding and removing (apache#8364)
  MINOR: reduce allocations in log start and recovery checkpoints (apache#8467)
  MINOR: Remove unused foreign-key join class (apache#8547)
  HOTFIX: Fix broker bounce system tests (apache#8532)
  KAFKA-9704: Fix the issue z/OS won't let us resize file when mmap. (apache#8224)
  KAFKA-8639: Replace AddPartitionsToTxn with Automated Protocol  (apache#8326)
  MINOR: equals() should compare all fields for generated classes (apache#8539)
  KAFKA-9844; Fix race condition which allows more than maximum number of members(apache#8454)
  KAFKA-9823: Remember the sent generation for the coordinator request (apache#8445)
  KAFKA-9883: Add better error message when REST API forwards a request and leader is not known (apache#8536)
  KAFKA-9907: Switch default build to Scala 2.13 (apache#8537)
  MINOR: Some html fixes in Streams DSL documentation (apache#8503)
  MINOR: Enable fatal warnings with scala 2.13 (apache#8429)
  KAFKA-9852: Change the max duration that calls to the buffer pool can block from 2000ms to 10ms to reduce overall test runtime (apache#8464)
  ...
ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 30, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into `confluentinc…
31c13ff 
…/master`

* apache-github/trunk: (45 commits)
  MINOR: Fix broken JMX link in docs by adding missing starting double quote (apache#8587)
  KAFKA-9652: Fix throttle metric in RequestChannel and request log due to KIP-219 (apache#8567)
  KAFKA-9922: Update demo instructions in examples README (apache#8559)
  KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses (apache#8442)
  KAFKA-9875: Make integration tests more resilient (apache#8578)
  KAFKA-9932: Don't load configs from ZK when the log has already been loaded (apache#8582)
  KAFKA-9925: decorate pseudo-topics with app id (apache#8574)
  KAFKA-9832: fix attempt to commit non-running tasks (apache#8580)
  KAFKA-9127: don't create StreamThreads for global-only topology (apache#8540)
  MINOR: add support for kafka 2.4 and 2.5 to downgrade test
  KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568)
  KAFKA-9823: Follow-up, check state for handling commit error response (apache#8548)
  KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541)
  MINOR: Fix partition numbering from 0 to P-1 instead of P in docs (apache#8572)
  KAFKA-9921: disable caching on stores configured to retain duplicates (apache#8564)
  Minor: remove redundant check in auto preferred leader election (apache#8566)
  MINOR: Update the link to the Raft paper in docs (apache#8560)
  MINOR: Fix typos in config properties in MM2 test (apache#8561)
  MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters. (apache#7982)
  MINOR: document how to escape json parameters to ducktape tests (apache#8546)
  ...
ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 30, 2020
@ijuma
CONFLUENT: Sync from apache/kafka (29 April 2020)
1f84e45 
There was a minor conflict in gradle.properties because the default
Scala version changed upstream to Scala 2.13. I kept the upstream
change.

Related to this, I have updated Jenkinsfile to compile and validate
with Scala 2.12 in a separate stage so that we ensure we maintain
compatibility. Unlike Apache Kafka, we only run the tests with the
default Scala version, which is now 2.13.

* apache-github/trunk: (45 commits)
MINOR: Fix broken JMX link in docs by adding missing starting double
quote (apache#8587)
KAFKA-9652: Fix throttle metric in RequestChannel and request log due
to KIP-219 (apache#8567)
  KAFKA-9922: Update demo instructions in examples README (apache#8559)
KAFKA-9830: Implement AutoCloseable in ErrorReporter and subclasses
(apache#8442)
  KAFKA-9875: Make integration tests more resilient (apache#8578)
KAFKA-9932: Don't load configs from ZK when the log has already been
loaded (apache#8582)
  KAFKA-9925: decorate pseudo-topics with app id (apache#8574)
  KAFKA-9832: fix attempt to commit non-running tasks (apache#8580)
KAFKA-9127: don't create StreamThreads for global-only topology
(apache#8540)
  MINOR: add support for kafka 2.4 and 2.5 to downgrade test
  KAFKA-9176: Retry on getting local stores from KafkaStreams (apache#8568)
KAFKA-9823: Follow-up, check state for handling commit error response
(apache#8548)
  KAFKA-6145: KIP-441: Add TaskAssignor class config (apache#8541)
MINOR: Fix partition numbering from 0 to P-1 instead of P in docs
(apache#8572)
KAFKA-9921: disable caching on stores configured to retain duplicates
(apache#8564)
Minor: remove redundant check in auto preferred leader election
(apache#8566)
  MINOR: Update the link to the Raft paper in docs (apache#8560)
  MINOR: Fix typos in config properties in MM2 test (apache#8561)
MINOR: Improve producer test BufferPoolTest#testCloseNotifyWaiters.
(apache#7982)
MINOR: document how to escape json parameters to ducktape tests
(apache#8546)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hachikuji hachikuji Awaiting requested review from hachikuji

@rajinisivaram rajinisivaram Awaiting requested review from rajinisivaram

1 more reviewer

@tedyu tedyu tedyu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bdbyrne @ijuma @tedyu @rajinisivaram