You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/10/12 12:23:21 UTC
[GitHub] [flink] StephanEwen opened a new pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
StephanEwen opened a new pull request #13593:
URL: https://github.com/apache/flink/pull/13593
## What is the purpose of the change
Fixes two test instabilities on the `SplitFetcherTest`.
## Brief change log
The test logic was previously incorrect. This now adjusts the logic to correctly reflect the requirements for idleness notifications, as required by the reader, where at least one of the following conditions must be true:
- Either the fetcher was already set to idle when the reader thread pulled the last fetch from the queue, because that is when the reader thread checks for fetcher being idle and shuts it down (which signals end of input)
- Or the fetcher was not yet marked idle, because the fetch was pulled before that flag was set. In that case, the availability future must be completed, so that the reader thread goes back to checking the fetcher.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] StephanEwen commented on a change in pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on a change in pull request #13593:
URL: https://github.com/apache/flink/pull/13593#discussion_r503411328
##########
File path: flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcherTest.java
##########
@@ -120,16 +120,16 @@ public void testNotifiesWhenGoingIdleConcurrent() throws Exception {
final SplitFetcher<Object, TestingSourceSplit> fetcher = createFetcherWithSplit(
"test-split", queue, new TestingSplitReader<>(finishedSplitFetch("test-split")));
- final QueueDrainerThread queueDrainer = new QueueDrainerThread(queue);
+ final QueueDrainerThread queueDrainer = new QueueDrainerThread(queue, fetcher, 1);
queueDrainer.start();
- try {
- fetcher.runOnce();
+ fetcher.runOnce();
- assertTrue(queue.getAvailabilityFuture().isDone());
- } finally {
- queueDrainer.shutdown();
- }
+ queueDrainer.sync();
+
+ // either we got the notification that the fetcher went idle after the queue was drained (thread finished)
+ // or the fetcher was already idle when the thread drained the queue (then we need no additional notification)
+ assertTrue(queue.getAvailabilityFuture().isDone() || queueDrainer.wasIdleWhenFinished());
Review comment:
yes, exactly. And this assertion is also exactly what the `SourceReaderBase` assumes. Either `isIdle` was true when it finished the poll and it could shut down the fetcher, or the availability future was complete, and the reader will be polled again.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707103718
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "75e101a5bebfa592eae0a3f2acaa4744dff5012e",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7444",
"triggerID" : "75e101a5bebfa592eae0a3f2acaa4744dff5012e",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 75e101a5bebfa592eae0a3f2acaa4744dff5012e Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7444)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707088549
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.
## Automated Checks
Last check on commit 75e101a5bebfa592eae0a3f2acaa4744dff5012e (Mon Oct 12 12:25:08 UTC 2020)
**Warnings:**
* No documentation files were touched! Remember to keep the Flink docs up to date!
<sub>Mention the bot in a comment to re-run the automated checks.</sub>
## Review Progress
* ❓ 1. The [description] looks good.
* ❓ 2. There is [consensus] that the contribution should go into to Flink.
* ❓ 3. Needs [attention] from.
* ❓ 4. The change fits into the overall [architecture].
* ❓ 5. Overall code [quality] is good.
Please see the [Pull Request Review Guide](https://flink.apache.org/contributing/reviewing-prs.html) for a full explanation of the review process.<details>
The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required <summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot approve description` to approve one or more aspects (aspects: `description`, `consensus`, `architecture` and `quality`)
- `@flinkbot approve all` to approve all aspects
- `@flinkbot approve-until architecture` to approve everything until `architecture`
- `@flinkbot attention @username1 [@username2 ..]` to require somebody's attention
- `@flinkbot disapprove architecture` to remove an approval you gave earlier
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] StephanEwen commented on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707226522
@kezhuw Do you think this fix is correct and can be merged?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707103718
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "75e101a5bebfa592eae0a3f2acaa4744dff5012e",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "75e101a5bebfa592eae0a3f2acaa4744dff5012e",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 75e101a5bebfa592eae0a3f2acaa4744dff5012e UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kezhuw commented on a change in pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
kezhuw commented on a change in pull request #13593:
URL: https://github.com/apache/flink/pull/13593#discussion_r503408507
##########
File path: flink-connectors/flink-connector-base/src/test/java/org/apache/flink/connector/base/source/reader/fetcher/SplitFetcherTest.java
##########
@@ -120,16 +120,16 @@ public void testNotifiesWhenGoingIdleConcurrent() throws Exception {
final SplitFetcher<Object, TestingSourceSplit> fetcher = createFetcherWithSplit(
"test-split", queue, new TestingSplitReader<>(finishedSplitFetch("test-split")));
- final QueueDrainerThread queueDrainer = new QueueDrainerThread(queue);
+ final QueueDrainerThread queueDrainer = new QueueDrainerThread(queue, fetcher, 1);
queueDrainer.start();
- try {
- fetcher.runOnce();
+ fetcher.runOnce();
- assertTrue(queue.getAvailabilityFuture().isDone());
- } finally {
- queueDrainer.shutdown();
- }
+ queueDrainer.sync();
+
+ // either we got the notification that the fetcher went idle after the queue was drained (thread finished)
+ // or the fetcher was already idle when the thread drained the queue (then we need no additional notification)
+ assertTrue(queue.getAvailabilityFuture().isDone() || queueDrainer.wasIdleWhenFinished());
Review comment:
Nice assertion. I think this assertion holds because of:
* Inside `SplitFetcher.checkAndSetIdle`, `isIdle = true` happens before `elementsQueue.notifyAvailable()`, and both are atomic.
* In queue drainer thread, if `queue.poll`, which is also atomic, happens before `elementsQueue.notifyAvailable()` then `queue.getAvailabilityFuture().isDone()` holds, otherwise, `queueDrainer.wasIdleWhenFinished()` holds.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] StephanEwen commented on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
StephanEwen commented on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707088500
@kezhuw This could be interesting to you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] kezhuw commented on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
kezhuw commented on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707230884
@StephanEwen Yes, from my side.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot edited a comment on pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #13593:
URL: https://github.com/apache/flink/pull/13593#issuecomment-707103718
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "75e101a5bebfa592eae0a3f2acaa4744dff5012e",
"status" : "FAILURE",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7444",
"triggerID" : "75e101a5bebfa592eae0a3f2acaa4744dff5012e",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 75e101a5bebfa592eae0a3f2acaa4744dff5012e Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=7444)
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run travis` re-run the last Travis build
- `@flinkbot run azure` re-run the last Azure build
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] asfgit closed pull request #13593: [FLINK-19427][FLINK-19489][tests] Fix test conditions for 'SplitFetcherTest.testNotifiesWhenGoingIdleConcurrent()'
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #13593:
URL: https://github.com/apache/flink/pull/13593
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org