You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "JayajP (via GitHub)" <gi...@apache.org> on 2023/12/18 23:31:51 UTC
[PR] Record mean for underflow/overflow values in HistogramData. [beam]
JayajP opened a new pull request, #29808:
URL: https://github.com/apache/beam/pull/29808
Record mean for underflow/overflow values in HistogramData.
User Workers will periodically export these values as part of the new 'per worker' BigQuery metrics.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://github.com/apache/beam/blob/master/CONTRIBUTING.md#make-the-reviewers-job-easier).
To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI or the [workflows README](https://github.com/apache/beam/blob/master/.github/workflows/README.md) to see a list of phrases to trigger workflows.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #29808:
URL: https://github.com/apache/beam/pull/29808#issuecomment-1874163174
Java PreCommit has been very flaky
failed tests:
```
testLogging (org.apache.beam.runners.fnexecution.control.RemoteExecutionTest)
testIsAvailableAlwaysWakenUp (org.apache.beam.runners.flink.translation.wrappers.streaming.io.source.unbounded.FlinkUnboundedSourceReaderTest)
testWithResume (org.apache.beam.runners.spark.translation.streaming.ResumeFromCheckpointStreamingTest)
testCloseVisibleToAwaitCompletionCallerAndProducer (org.apache.beam.sdk.fn.data.BeamFnDataInboundObserverTest)
```
run again to see if its relevant to this change
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "JayajP (via GitHub)" <gi...@apache.org>.
JayajP commented on code in PR #29808:
URL: https://github.com/apache/beam/pull/29808#discussion_r1437852114
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/util/HistogramDataTest.java:
##########
@@ -357,27 +357,74 @@ public void testStatistics_sumOfSquaredDeviations() {
@Test
public void testGetAndReset_resetSucceeds() {
- HistogramData originalHistogram = HistogramData.linear(0, 10, 10);
+ // Records values from [20, 50) in three buckets that have width 10.
+ HistogramData originalHistogram = HistogramData.linear(20, 10, 3);
originalHistogram.record(15.0, 25.0, 35.0, 45.0);
originalHistogram.getAndReset();
- HistogramData emptyHistogramData = HistogramData.linear(0, 10, 10);
+ HistogramData emptyHistogramData = HistogramData.linear(20, 10, 3);
Review Comment:
The spacing for `originalHistogram` and `emptyHistogramData` need to be equal. If I used the original spacing for both histograms then it would still pass the test; however, I wanted to ensure these tests were incrementing the underflow/overflow buckets.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on code in PR #29808:
URL: https://github.com/apache/beam/pull/29808#discussion_r1437249953
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java:
##########
@@ -231,6 +239,38 @@ private synchronized void updateStatistics(double value) {
sumOfSquaredDeviations += (value - mean) * (value - oldMean);
}
+ /**
+ * Increment the {@code numTopRecords} and update {@code topRecordsMean} when a new overflow value
+ * is recorded. This function should only be called when a Histogram is recording a value greater
+ * than the upper bound of it's largest bucket.
+ *
+ * @param value
+ */
+ private synchronized void recordTopRecordsValue(double value) {
+ numTopRecords++;
+ if (numTopRecords == 1) {
+ topRecordsMean = value;
+ } else {
+ topRecordsMean = topRecordsMean + (value - topRecordsMean) / numTopRecords;
Review Comment:
Can we simply add up value every time, that is use "topRecordSum", and calculate the mean by sum/counter when needed. As of floating point arithmetics, this formula can lose precision over time. Also division is more expensive than addition
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/util/HistogramDataTest.java:
##########
@@ -357,27 +357,74 @@ public void testStatistics_sumOfSquaredDeviations() {
@Test
public void testGetAndReset_resetSucceeds() {
- HistogramData originalHistogram = HistogramData.linear(0, 10, 10);
+ // Records values from [20, 50) in three buckets that have width 10.
+ HistogramData originalHistogram = HistogramData.linear(20, 10, 3);
originalHistogram.record(15.0, 25.0, 35.0, 45.0);
originalHistogram.getAndReset();
- HistogramData emptyHistogramData = HistogramData.linear(0, 10, 10);
+ HistogramData emptyHistogramData = HistogramData.linear(20, 10, 3);
Review Comment:
Is there a reason that needs to change these parameters? The original spacing should still pass the test, If not does it indicate some issue?
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #29808:
URL: https://github.com/apache/beam/pull/29808#issuecomment-1861924591
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn merged PR #29808:
URL: https://github.com/apache/beam/pull/29808
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "JayajP (via GitHub)" <gi...@apache.org>.
JayajP commented on code in PR #29808:
URL: https://github.com/apache/beam/pull/29808#discussion_r1437852224
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java:
##########
@@ -231,6 +239,38 @@ private synchronized void updateStatistics(double value) {
sumOfSquaredDeviations += (value - mean) * (value - oldMean);
}
+ /**
+ * Increment the {@code numTopRecords} and update {@code topRecordsMean} when a new overflow value
+ * is recorded. This function should only be called when a Histogram is recording a value greater
+ * than the upper bound of it's largest bucket.
+ *
+ * @param value
+ */
+ private synchronized void recordTopRecordsValue(double value) {
+ numTopRecords++;
+ if (numTopRecords == 1) {
+ topRecordsMean = value;
+ } else {
+ topRecordsMean = topRecordsMean + (value - topRecordsMean) / numTopRecords;
Review Comment:
Done.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Record mean for underflow/overflow values in HistogramData. [beam]
Posted by "Abacn (via GitHub)" <gi...@apache.org>.
Abacn commented on PR #29808:
URL: https://github.com/apache/beam/pull/29808#issuecomment-1874329616
Some tests in java-fn-execution stucks indefinitely
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org