Skip to content

[BEAM-12755] Stop throwing exceptions during formatting and calculation of a percentile #15329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lukecwik merged 1 commit into from
Aug 17, 2021
Merged

[BEAM-12755] Stop throwing exceptions during formatting and calculation of a percentile #15329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 6 additions & 12 deletions sdks/java/core/src/main/java/org/apache/beam/sdk/util/HistogramData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,8 @@ public synchronized long getTotalCount() {

public synchronized String getPercentileString(String elemType, String unit) {
return String.format(
"Total number of %s: %s, P99: %s%s, P90: %s%s, P50: %s%s",
elemType,
getTotalCount(),
DoubleMath.roundToInt(p99(), RoundingMode.HALF_UP),
unit,
DoubleMath.roundToInt(p90(), RoundingMode.HALF_UP),
unit,
DoubleMath.roundToInt(p50(), RoundingMode.HALF_UP),
unit);
"Total number of %s: %s, P99: %.0f %s, P90: %.0f %s, P50: %.0f %s",
elemType, getTotalCount(), p99(), unit, p90(), unit, p50(), unit);
}

/**
Expand Down Expand Up @@ -196,7 +189,7 @@ public double p50() {
private synchronized double getLinearInterpolation(double percentile) {
long totalNumOfRecords = getTotalCount();
if (totalNumOfRecords == 0) {
Copy link

@ajamato ajamato Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this code shouldn't ever be called if totalNumOfRecords == 0
This feature is only used to print some info logging.
So ti could just print something else in this case

WDYT @ihji ?

throw new RuntimeException("histogram has no record.");
Copy link

@ajamato ajamato Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious. Did you decide to make this change because you saw this exception thrown in a user pipeline? (Its possible the code is designed to not invoke this method in this case). Just wondering if actual pipelines are hitting this?

Or did you just decide to make this change to make to remove the exception, make the code safer, etc.

Copy link
Member Author

@lukecwik lukecwik Aug 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User pipelines were hitting this, b/189940287

return Double.NaN;
}
int index;
double recordSum = numBottomRecords;
Expand Down Expand Up @@ -245,10 +238,11 @@ public abstract static class LinearBuckets implements BucketType {

public static LinearBuckets of(double start, double width, int numBuckets) {
if (width <= 0) {
throw new RuntimeException(String.format("width should be greater than zero: %f", width));
throw new IllegalArgumentException(
String.format("width should be greater than zero: %f", width));
}
if (numBuckets <= 0) {
throw new RuntimeException(
throw new IllegalArgumentException(
String.format("numBuckets should be greater than zero: %d", numBuckets));
}
return new AutoValue_HistogramData_LinearBuckets(start, width, numBuckets);
Expand Down
10 changes: 8 additions & 2 deletions sdks/java/core/src/test/java/org/apache/beam/sdk/util/HistogramDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertThrows;

import org.junit.Test;
import org.junit.runner.RunWith;
Expand Down Expand Up @@ -120,19 +119,26 @@ public void testP50NegativeInfinity() {
HistogramData histogramData = HistogramData.linear(0, 0.2, 50);
histogramData.record(-1, -2, -3, -4, -5, 0, 1, 2, 3, 4);
assertThat(histogramData.p50(), equalTo(Double.NEGATIVE_INFINITY));
assertThat(
histogramData.getPercentileString("meows", "cats"),
equalTo("Total number of meows: 10, P99: 4 cats, P90: 3 cats, P50: -Infinity cats"));
}

@Test
public void testP50PositiveInfinity() {
HistogramData histogramData = HistogramData.linear(0, 0.2, 50);
histogramData.record(6, 7, 8, 9, 10, 11, 12, 13, 14, 15);
assertThat(histogramData.p50(), equalTo(Double.POSITIVE_INFINITY));
assertThat(
histogramData.getPercentileString("meows", "cats"),
equalTo(
"Total number of meows: 10, P99: Infinity cats, P90: Infinity cats, P50: Infinity cats"));
}

@Test
public void testEmptyP99() {
HistogramData histogramData = HistogramData.linear(0, 0.2, 50);
assertThrows(RuntimeException.class, histogramData::p99);
assertThat(histogramData.p99(), equalTo(Double.NaN));
}

@Test
Expand Down