MINOR: Handle expandIsr in PartitionLockTest and ensure read threads not blocked on write#7973
Merged
rajinisivaram merged 2 commits intoapache:trunkfrom Jan 17, 2020
Conversation
…not blocked on write
hachikuji
reviewed
Jan 17, 2020
Contributor
hachikuji
left a comment
hachikuji
left a comment
There was a problem hiding this comment.
Thanks @rajinisivaram. This looks good. Left some minor comments.
| stateUpdateFutures.foreach(_.get(15, TimeUnit.SECONDS)) | ||
|
|
||
| appendSemaphore.release(1) | ||
| scheduleUpdateFollowers(1).foreach(_.get(15, TimeUnit.SECONDS)) |
Contributor
There was a problem hiding this comment.
This is just ensuring there is one call to updateFollowers with every append?
Contributor
Author
There was a problem hiding this comment.
yes, it isn't really necessary for the test, but kept it anyway and added a comment.
| * Verify that follower state updates complete even though an append holding read lock is in progress. | ||
| * Then release the permit for the final append and verify that all appends and follower updates complete. | ||
| */ | ||
| private def concurrentProduceFetchWithReadLockOnly(appendSemaphore: Semaphore): Unit = { |
Contributor
There was a problem hiding this comment.
nit: would be nice to be consistent on the usage of class fields. This wouldn't work if the passed semaphore wasn't the class field since that is what we used when building the log. Maybe we can drop the parameter?
|
|
||
| assertFalse(stateUpdateFutures.exists(_.isDone)) | ||
| shrinkIsrSemaphore.release() | ||
| appendSemaphore.release(numProducers * numRecordsPerProducer) |
Contributor
There was a problem hiding this comment.
Just checking my understanding, but it seems there's no need to do this after the shrink semaphore is released. If we did it before, then we could assert that the append futures are blocked just like the update follower futures.
Contributor
Author
There was a problem hiding this comment.
Good idea, updated.
Contributor
Author
|
@hachikuji Thanks for the review, addressed the comments. |
Contributor
Author
|
retest this please |
Contributor
|
retest this please |
Contributor
Author
|
@hachikuji Thanks for the review, merging to trunk. |
ijuma
added a commit
to confluentinc/kafka
that referenced
this pull request
Jan 21, 2020
Conflicts or compilation errors due to the fact that we temporarily reverted the commit that removes Scala 2.11 support: * AclCommand.scala: take upstream changes. * AclCommandTest.scala: take upstream changes. * TransactionCoordinatorTest.scala: don't use SAMs, but adjust mock call to putTransactionStateIfNotExists given new signature. * TransactionStateManagerTest: use Runnable instead of SAMs. * PartitionLockTest: use Runnable instead of SAMs. * docs/upgrade.html: take upstream changes excluding line that states that Scala 2.11 support has been removed. * apache-github/trunk: (28 commits) KAFKA-9457; Fix flaky test org.apache.kafka.common.network.SelectorTest.testGracefulClose (apache#7989) MINOR: Update AclCommand help message to match implementation (apache#7990) MINOR: Update introduction page in Kafka documentation MINOR: Use Math.min for StreamsPartitionAssignor#updateMinReceivedVersion method (apache#7954) KAFKA-9338; Fetch session should cache request leader epoch (apache#7970) KAFKA-9329; KafkaController::replicasAreValid should return error message (apache#7865) KAFKA-9449; Adds support for closing the producer's BufferPool. (apache#7967) MINOR: Handle expandIsr in PartitionLockTest and ensure read threads not blocked on write (apache#7973) MINOR: Fix typo in connect integration test class name (apache#7976) KAFKA-9218: MirrorMaker 2 can fail to create topics (apache#7745) KAFKA-8847; Deprecate and remove usage of supporting classes in kafka.security.auth (apache#7966) MINOR: Suppress DescribeConfigs Denied log during CreateTopics (apache#7971) [MINOR]: Fix typo in Fetcher comment (apache#7934) MINOR: Remove unnecessary call to `super` in `MetricConfig` constructor (apache#7975) MINOR: fix flaky StreamsUpgradeTestIntegrationTest (apache#7974) KAFKA-9431: Expose API in KafkaStreams to fetch all local offset lags (apache#7961) KAFKA-9235; Ensure transaction coordinator is stopped after replica deletion (apache#7963) KAFKA-9410; Make groupId Optional in KafkaConsumer (apache#7943) MINOR: Removed accidental double negation in error message. (apache#7834) KAFKA-6144: IQ option to query standbys (apache#7962) ...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Noticed a PR build failure in the new PartitionLockTest:
The mocked instance was not handling
expandIsr, so updated the test to handle this. Also updated the test to use the offset from the batch to trigger expandIsr more often. With this change, the test may try to acquire write lock while appends are blocked on read lock, so separated out the test using write lock to make it safer.Committer Checklist (excluded from commit message)