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 2022/10/12 13:11:30 UTC
[GitHub] [flink] echauchot opened a new pull request, #21037: [FLINK-29563] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
echauchot opened a new pull request, #21037:
URL: https://github.com/apache/flink/pull/21037
## What is the purpose of the change
Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Based on [timeout pr](https://github.com/apache/flink/pull/21035).
## Brief change log
make SourceTestSuiteBase#testSourceMetrics compatible with both bounded and unbounded sources and add a boundedness parameter to the test to allow the implemeter to pass the corresponding boundedness by overriding this test in the ITCase of its source. Default (without override) is unbounded mode for conservative reasons.
## Verifying this change
This change is already covered by existing tests
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? yes test feature
- If yes, how is the feature documented? javadoc
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1282298514
> > What is the exception that you? Is it the assertion error about the final job state? Why not just allow both canceled & finished?
>
> While testing an incorrect bounded source that does not report the expected metric (source under dev), running this test without disabling the killJob, here is what happens:
>
> 1. the timeout fires
> 2. the finally bloc runs the `killJob()` method that runs `CommonTestUtils.terminateJob() ` that triggers the job cancelation
> 3. `Dispatcher#cancelJob()` throws `org.apache.flink.runtime.messages.FlinkJobTerminatedWithoutCancellationException: Flink job (8e3ca8038550e7e5aa3018b887ebf9a2) was not canceled, but instead FINISHED.`
>
> So my reasoning was: on a bounded source, there is no point in killing the job as it will eventually finish. This will avoid the problem above. So when testing this incorrect source now we get j`ava.util.concurrent.TimeoutException: Timeout while comparing source metrics with 80 expected value` which is precisely the root cause of the problem in the source and not the `FlinkJobTerminatedWithoutCancellationException`
@zentol WDYT ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot closed pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot closed pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
URL: https://github.com/apache/flink/pull/21037
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on a diff in pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21037:
URL: https://github.com/apache/flink/pull/21037#discussion_r993503365
##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -462,11 +477,18 @@ public void testSourceMetrics(
// skip failed assert try
return false;
}
- });
+ },
+ METRICS_TIMEOUT,
+ DELAY_BETWEEN_CHECKS,
+ String.format(
+ "Timeout while comparing source metrics with %d expected value",
+ getTestDataSize(testRecordCollections)));
Review Comment:
And by the way, this change you're commenting belongs to [the timeout pr](https://github.com/apache/flink/pull/21035) on which this PR is based. We should discuss it there.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on a diff in pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21037:
URL: https://github.com/apache/flink/pull/21037#discussion_r993503365
##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -462,11 +477,18 @@ public void testSourceMetrics(
// skip failed assert try
return false;
}
- });
+ },
+ METRICS_TIMEOUT,
+ DELAY_BETWEEN_CHECKS,
+ String.format(
+ "Timeout while comparing source metrics with %d expected value",
+ getTestDataSize(testRecordCollections)));
Review Comment:
And by the way, this change you're commenting belongs to [the timeout pr](https://github.com/apache/flink/pull/21035) on which the current PR is based. The current PR is about adding a bounded test so we should discuss the timeout in the other PR.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1282534029
@flinkbot run azure
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1276320564
> What is the exception that you? Is it the assertion error about the final job state? Why not just allow both canceled & finished?
While testing an incorrect bounded source that does not report the expected metric (under dev), running this test without disabling the killJob, here is what happens:
1. the timeout fires
2. the finally bloc runs the `killJob()` method that runs `CommonTestUtils.terminateJob() ` that triggers the job cancelation
3. `Dispatcher#cancelJob()` throws `org.apache.flink.runtime.messages.FlinkJobTerminatedWithoutCancellationException: Flink job (8e3ca8038550e7e5aa3018b887ebf9a2) was not canceled, but instead FINISHED.`
So my reasoning was: on a bounded source, there is no point in killing the job as it will eventually finish. This will avoid the problem above.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1294639861
> ## CI report:
> * [4505e0c](https://github.com/apache/flink/commit/4505e0c5d6384a7faadb84d98b7339bf9bbbb527) Azure: [FAILURE](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=41947)
>
> Bot commands
Strange thing. I have set a timeout of 30s and it is hit from time to time by pulsar E2E tests. This means that in some cases metrics retrieval in `testSourceMetrics` is either too long (more than 30s) or the counter of reported source metrics is incorrect leading to infinite wait on the counter condition.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #21037:
URL: https://github.com/apache/flink/pull/21037#discussion_r993448262
##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -462,11 +477,18 @@ public void testSourceMetrics(
// skip failed assert try
return false;
}
- });
+ },
+ METRICS_TIMEOUT,
+ DELAY_BETWEEN_CHECKS,
+ String.format(
+ "Timeout while comparing source metrics with %d expected value",
+ getTestDataSize(testRecordCollections)));
Review Comment:
We're trying to get rid of timeouts in tests.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on a diff in pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21037:
URL: https://github.com/apache/flink/pull/21037#discussion_r993500768
##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -462,11 +477,18 @@ public void testSourceMetrics(
// skip failed assert try
return false;
}
- });
+ },
+ METRICS_TIMEOUT,
+ DELAY_BETWEEN_CHECKS,
+ String.format(
+ "Timeout while comparing source metrics with %d expected value",
+ getTestDataSize(testRecordCollections)));
Review Comment:
sure but if an implementor is testing his buggy source while developing it (like I did), it would be better if this test fails with the timeout rather than running into an infinite loop, no ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on a diff in pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21037:
URL: https://github.com/apache/flink/pull/21037#discussion_r993500768
##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -462,11 +477,18 @@ public void testSourceMetrics(
// skip failed assert try
return false;
}
- });
+ },
+ METRICS_TIMEOUT,
+ DELAY_BETWEEN_CHECKS,
+ String.format(
+ "Timeout while comparing source metrics with %d expected value",
+ getTestDataSize(testRecordCollections)));
Review Comment:
sure but if an implementor is testing his buggy source while developing it (like I did), it would be better if this test fails with the timeout rather than running into an infinite loop, no ?
but still we could do otherwise if you want for example: if the counter fetched by the rest call is still 0 after some attempts we could throw an exception in checkSourceMetrics.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1276166901
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "4505e0c5d6384a7faadb84d98b7339bf9bbbb527",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "4505e0c5d6384a7faadb84d98b7339bf9bbbb527",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 4505e0c5d6384a7faadb84d98b7339bf9bbbb527 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1290617330
@flinkbot run azure
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] echauchot commented on pull request #21037: [FLINK-29605] Allow implementers to have bounded and unbounded SourceTestSuiteBase#testSourceMetrics
Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot commented on PR #21037:
URL: https://github.com/apache/flink/pull/21037#issuecomment-1431016205
I think `SourceTestSuiteBase#testSourceMetrics` is defined as a streaming test. Closing this PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org