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 12:34:17 UTC

[GitHub] [flink] echauchot opened a new pull request, #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

echauchot opened a new pull request, #21035:
URL: https://github.com/apache/flink/pull/21035

   ## What is the purpose of the change
   
   * Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics *
   
   
   ## Brief change log
   
   use `flink.core.testutils.CommonTestUtils.waitUtil(condition, timeout, delay, message)` instead of `flink.runtime.testutils.CommonTestUtils.waitUntilCondition(condition)` in `SourceTestSuiteBase#testSourceMetrics`
   
   
   ## 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? no
     - If yes, how is the feature documented? not applicable
   


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1494047977

   @tzulitai 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 commented on pull request #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1282071735

   linking to the timeout discussion on the other PR: https://github.com/apache/flink/pull/21037#discussion_r993448262


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1276101855

   <!--
   Meta data
   {
     "version" : 1,
     "metaDataEntries" : [ {
       "hash" : "56c44bd4e3ef6ed5e3e65d12079f1fd257c3cc57",
       "status" : "UNKNOWN",
       "url" : "TBD",
       "triggerID" : "56c44bd4e3ef6ed5e3e65d12079f1fd257c3cc57",
       "triggerType" : "PUSH"
     } ]
   }-->
   ## CI report:
   
   * 56c44bd4e3ef6ed5e3e65d12079f1fd257c3cc57 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 a diff in pull request #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21035:
URL: https://github.com/apache/flink/pull/21035#discussion_r993466238


##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -448,7 +451,7 @@ public void testSourceMetrics(
                                     testEnv.getRestEndpoint(),
                                     jobClient.getJobID()));
 
-            waitUntilCondition(
+            waitUtil(

Review Comment:
   too low (0) because of the bug in my Cassandra source implementation I mentioned (privately): the intermediate type I used in the source did not contain a single record but multiple ones.  
   
   with this timeout fix (that should not be hit with a fixed Cassandra source) + bounded test version (https://github.com/apache/flink/pull/21037) `testSourceMetrics` passes on my Cassandra source (PR to come soon)



-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1282532020

   @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 a diff in pull request #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21035:
URL: https://github.com/apache/flink/pull/21035#discussion_r993466238


##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -448,7 +451,7 @@ public void testSourceMetrics(
                                     testEnv.getRestEndpoint(),
                                     jobClient.getJobID()));
 
-            waitUntilCondition(
+            waitUtil(

Review Comment:
   too low (0) because of the bug in my Cassandra source implementation I mentioned (privately): the intermediate type I used in the source did not contain a single record but multiple ones.  
   
   with this timeout fix + bounded test version (https://github.com/apache/flink/pull/21037) `testSourceMetrics` passes on my Cassandra source (PR to come soon)



-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1497504643

   > @zentol what do you want that we do with this PR? Having no safe guard in the test waiting loop seems dangerous to me: during the implementation of a source, it could happen that the metrics counter is incorrect leading to infinite wait. IMHO I think we should merge this PR or with an alternative stop condition if we don't want the timeout.
   
   @tzulitai I'm a bit confused here, I know that we are trying to avoid timeouts as much as possible but in this particular case a timeout is still better than an infinite waiting loop in case of incorrect metrics. 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 commented on pull request #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1327525295

   @zentol what do you want that we do with this PR? 
   Having no safe guard in the test waiting loop seems dangerous to me: during the implementation of a source, it could happen that the metrics counter is incorrect leading to infinite wait. IMHO I think we should merge this PR or with an alternative stop condition if we don't want the timeout.


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1364030205

   @zentol friendly ping


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot closed pull request #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics
URL: https://github.com/apache/flink/pull/21035


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on code in PR #21035:
URL: https://github.com/apache/flink/pull/21035#discussion_r993466238


##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -448,7 +451,7 @@ public void testSourceMetrics(
                                     testEnv.getRestEndpoint(),
                                     jobClient.getJobID()));
 
-            waitUntilCondition(
+            waitUtil(

Review Comment:
   too low (0) because of the bug in my Cassandra source implementation I mentioned (privately): the intermediate type I used in the source did not contain a single record but multiple ones.  



-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1282276674

   > linking to the timeout discussion on the other PR: [#21037 (comment)](https://github.com/apache/flink/pull/21037#discussion_r993448262)
   
   @zentol you wanted to avoid using a timeout. Do you want that I change the timeout to a number of retries as proposed 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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1457801466

   @MartijnVisser, I'm resuming this PR that has been stale for a log time.
   Do you have any thoughts about the discussion here ?


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by "echauchot (via GitHub)" <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1498635285

   Closing because is goes against the no timeout current philosophy :shrug: 


-- 
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] MartijnVisser commented on pull request #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by "MartijnVisser (via GitHub)" <gi...@apache.org>.
MartijnVisser commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1481949504

   @tzulitai Also any thoughts on this one?


-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #21035:
URL: https://github.com/apache/flink/pull/21035#discussion_r993453532


##########
flink-test-utils-parent/flink-connector-test-utils/src/main/java/org/apache/flink/connector/testframe/testsuites/SourceTestSuiteBase.java:
##########
@@ -448,7 +451,7 @@ public void testSourceMetrics(
                                     testEnv.getRestEndpoint(),
                                     jobClient.getJobID()));
 
-            waitUntilCondition(
+            waitUtil(

Review Comment:
   What was the actual issue in your test? Was your count to low, or too high?
   



-- 
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 #21035: [FLINK-29563] Add a timeout in the wait of metrics counter in SourceTestSuiteBase#testSourceMetrics

Posted by GitBox <gi...@apache.org>.
echauchot commented on PR #21035:
URL: https://github.com/apache/flink/pull/21035#issuecomment-1290617059

   @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