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/03/24 09:49:08 UTC
[GitHub] [flink] rkhachatryan opened a new pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
rkhachatryan opened a new pull request #19222:
URL: https://github.com/apache/flink/pull/19222
There is a race condition in ChangelogStorageMetricsTest.testAttemptsPerUpload:
- the assertion is made as soon as upload (future) is completed
- the histogram is updated after completing the upload (on success)
Moving assertion out of try/close block solves the problem.
--
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] rkhachatryan commented on a change in pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on a change in pull request #19222:
URL: https://github.com/apache/flink/pull/19222#discussion_r834215167
##########
File path: flink-dstl/flink-dstl-dfs/src/test/java/org/apache/flink/changelog/fs/ChangelogStorageMetricsTest.java
##########
@@ -204,12 +204,12 @@ void testAttemptsPerUpload() throws Exception {
writer.append(0, new byte[] {0, 1, 2, 3});
writer.persist(from).get();
}
- HistogramStatistics histogram = metrics.getAttemptsPerUpload().getStatistics();
- assertThat(histogram.getMin()).isEqualTo(maxAttempts);
- assertThat(histogram.getMax()).isEqualTo(maxAttempts);
} finally {
storage.close();
}
+ HistogramStatistics histogram = metrics.getAttemptsPerUpload().getStatistics();
+ assertThat(histogram.getMin()).isEqualTo(maxAttempts);
+ assertThat(histogram.getMax()).isEqualTo(maxAttempts);
Review comment:
Moving the assertions after `close` requires the thread that finished the upload to stop, i.e. it must have completed handling the upload and updated the metrics.
--
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] curcur commented on a change in pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
curcur commented on a change in pull request #19222:
URL: https://github.com/apache/flink/pull/19222#discussion_r834422606
##########
File path: flink-dstl/flink-dstl-dfs/src/test/java/org/apache/flink/changelog/fs/ChangelogStorageMetricsTest.java
##########
@@ -204,12 +204,12 @@ void testAttemptsPerUpload() throws Exception {
writer.append(0, new byte[] {0, 1, 2, 3});
writer.persist(from).get();
}
- HistogramStatistics histogram = metrics.getAttemptsPerUpload().getStatistics();
- assertThat(histogram.getMin()).isEqualTo(maxAttempts);
- assertThat(histogram.getMax()).isEqualTo(maxAttempts);
} finally {
storage.close();
}
+ HistogramStatistics histogram = metrics.getAttemptsPerUpload().getStatistics();
+ assertThat(histogram.getMin()).isEqualTo(maxAttempts);
+ assertThat(histogram.getMax()).isEqualTo(maxAttempts);
Review comment:
Thanks for the explanation!
--
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] rkhachatryan merged pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
rkhachatryan merged pull request #19222:
URL: https://github.com/apache/flink/pull/19222
--
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 #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
flinkbot commented on pull request #19222:
URL: https://github.com/apache/flink/pull/19222#issuecomment-1077442461
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "8a63fb6db4ba97484c2bc33695031f2f2e7e62f7",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "8a63fb6db4ba97484c2bc33695031f2f2e7e62f7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 8a63fb6db4ba97484c2bc33695031f2f2e7e62f7 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] flinkbot edited a comment on pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #19222:
URL: https://github.com/apache/flink/pull/19222#issuecomment-1077442461
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "8a63fb6db4ba97484c2bc33695031f2f2e7e62f7",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=33691",
"triggerID" : "8a63fb6db4ba97484c2bc33695031f2f2e7e62f7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 8a63fb6db4ba97484c2bc33695031f2f2e7e62f7 Azure: [SUCCESS](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=33691)
<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] flinkbot edited a comment on pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
flinkbot edited a comment on pull request #19222:
URL: https://github.com/apache/flink/pull/19222#issuecomment-1077442461
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "8a63fb6db4ba97484c2bc33695031f2f2e7e62f7",
"status" : "PENDING",
"url" : "https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=33691",
"triggerID" : "8a63fb6db4ba97484c2bc33695031f2f2e7e62f7",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 8a63fb6db4ba97484c2bc33695031f2f2e7e62f7 Azure: [PENDING](https://dev.azure.com/apache-flink/98463496-1af2-4620-8eab-a2ecc1a2e6fe/_build/results?buildId=33691)
<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] curcur commented on a change in pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
curcur commented on a change in pull request #19222:
URL: https://github.com/apache/flink/pull/19222#discussion_r834206728
##########
File path: flink-dstl/flink-dstl-dfs/src/test/java/org/apache/flink/changelog/fs/ChangelogStorageMetricsTest.java
##########
@@ -204,12 +204,12 @@ void testAttemptsPerUpload() throws Exception {
writer.append(0, new byte[] {0, 1, 2, 3});
writer.persist(from).get();
}
- HistogramStatistics histogram = metrics.getAttemptsPerUpload().getStatistics();
- assertThat(histogram.getMin()).isEqualTo(maxAttempts);
- assertThat(histogram.getMax()).isEqualTo(maxAttempts);
} finally {
storage.close();
}
+ HistogramStatistics histogram = metrics.getAttemptsPerUpload().getStatistics();
+ assertThat(histogram.getMin()).isEqualTo(maxAttempts);
+ assertThat(histogram.getMax()).isEqualTo(maxAttempts);
Review comment:
Would you please explain why moving the assert out can make sure that
the histogram is **guaranteed** updated?
--
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] rkhachatryan commented on pull request #19222: [FLINK-26809][tests] Assert histogram state after stopping the uploader
Posted by GitBox <gi...@apache.org>.
rkhachatryan commented on pull request #19222:
URL: https://github.com/apache/flink/pull/19222#issuecomment-1078679614
Thanks for the review! Merged into master and 1.15.
--
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