From 3182dcd946010c4921a233b2847677d5da907275 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 10 Oct 2023 15:33:56 -0400 Subject: [PATCH] clean up remaning exceptions --- docs/changelog/100368.yaml | 2 +- .../aggregations/AdaptingAggregator.java | 1 - .../aggregations/AggregationErrors.java | 24 ++++++++++++++----- .../AggregationExecutionException.java | 21 ++++++++++++++++ .../search/aggregations/InternalOrder.java | 3 +-- .../bucket/BucketsAggregator.java | 18 +++++--------- .../global/GlobalAggregatorFactory.java | 2 +- .../bucket/prefix/IpPrefixAggregator.java | 12 ++-------- .../GlobalOrdinalsStringTermsAggregator.java | 2 +- .../BucketMetricsPipelineAggregator.java | 8 +++---- ...mulativeCardinalityPipelineAggregator.java | 5 ++-- 11 files changed, 57 insertions(+), 41 deletions(-) diff --git a/docs/changelog/100368.yaml b/docs/changelog/100368.yaml index f50d84568eb38..534b780498df5 100644 --- a/docs/changelog/100368.yaml +++ b/docs/changelog/100368.yaml @@ -1,5 +1,5 @@ pr: 100368 summary: "Status codes for Aggregation errors, part 2" area: Aggregations -type: tech debt +type: Enhancement issues: [] diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java index 6888fbeef9510..0be4e7f729bbf 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java @@ -112,7 +112,6 @@ public final InternalAggregation buildEmptyAggregation() { return adapt(delegate.buildEmptyAggregation()); } catch (IOException e) { // We don't expect this to happen, but computers are funny. - // 500 class error, because we believe it to be unreachable? throw new AggregationExecutionException("io error while building empty agg", e); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationErrors.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationErrors.java index bfe74cbbdbd4d..6749929075929 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationErrors.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationErrors.java @@ -8,7 +8,6 @@ package org.elasticsearch.search.aggregations; -import org.elasticsearch.search.aggregations.pipeline.AbstractPipelineAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSource; import java.util.Optional; @@ -126,20 +125,33 @@ public static RuntimeException unsupportedRounding(String typeName) { * Indicates that an aggregation path (e.g. from a pipeline agg) references an aggregation of the wrong type, for example * attempting to take a cumulative cardinality of something other than a cardinality aggregation. * - * @param aggPath the path element found to be invalid - * @param got What we actually got; this may be null. + * @param aggPath the path element found to be invalid + * @param expected + * @param got What we actually got; this may be null. * @param currentAgg The name of the aggregation in question * @return an appropriate exception */ - public static RuntimeException incompatibleAggregationType(String aggPath, String got, String currentAgg) { + public static RuntimeException incompatibleAggregationType(String aggPath, String expected, String got, String currentAgg) { - return new IllegalArgumentException( + return new AggregationExecutionException.InvalidBucketPath( aggPath - + " must reference a cardinality aggregation, got: [" + + " must reference a " + expected + " aggregation, got: [" + (got == null ? "null" : got) + "] at aggregation [" + currentAgg + "]" ); } + + /** + * This is a 500 class error indicating a programming error. Hopefully we never see this outside of tests. + * @param bucketOrds - the ords we are processing + * @param got - the ordinal we got + * @param expected - the ordinal we expected + * @return an appropriate exception + */ + public static RuntimeException iterationOrderChangedWithoutMutating(String bucketOrds, long got, long expected) { + return new AggregationExecutionException( + "Iteration order of [" + bucketOrds + "] changed without mutating. [" + got + "] should have been [" + expected + "]" ); + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionException.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionException.java index 2b83979673e3b..42934447ff9d8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionException.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationExecutionException.java @@ -9,6 +9,7 @@ import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.rest.RestStatus; import java.io.IOException; @@ -28,4 +29,24 @@ public AggregationExecutionException(String msg, Throwable cause) { public AggregationExecutionException(StreamInput in) throws IOException { super(in); } + + public static class InvalidBucketPath extends AggregationExecutionException { + + public InvalidBucketPath(String msg) { + super(msg); + } + + public InvalidBucketPath(String msg, Throwable cause) { + super(msg, cause); + } + + public InvalidBucketPath(StreamInput in) throws IOException { + super(in); + } + + @Override + public RestStatus status() { + return RestStatus.BAD_REQUEST; + } + } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java b/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java index 6f7f0c6659606..995dc018f5171 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java @@ -65,8 +65,7 @@ public Comparator partiallyBuiltBucketComparator(ToLongFun BucketComparator bucketComparator = path.bucketComparator(aggregator, order); return (lhs, rhs) -> bucketComparator.compare(ordinalReader.applyAsLong(lhs), ordinalReader.applyAsLong(rhs)); } catch (IllegalArgumentException e) { - // Seems like this should be 400 (non-retryable), but we clearly intentionally throw a 500 here. Why? - throw new AggregationExecutionException("Invalid aggregation order path [" + path + "]. " + e.getMessage(), e); + throw new IllegalArgumentException("Invalid aggregation order path [" + path + "]. " + e.getMessage(), e); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java index caa82ee9bf5df..2a87783211fe1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java @@ -11,6 +11,7 @@ import org.elasticsearch.common.breaker.CircuitBreaker; import org.elasticsearch.common.util.LongArray; import org.elasticsearch.core.Releasable; +import org.elasticsearch.search.aggregations.AggregationErrors; import org.elasticsearch.search.aggregations.AggregationExecutionException; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorBase; @@ -340,8 +341,9 @@ protected final InternalAggregation[] buildAggregationsForVariableBuckets( totalOrdsToCollect += bucketCount; } if (totalOrdsToCollect > Integer.MAX_VALUE) { - // Seems like this should be a 400; also, should this check the bucket limit instead of Integer.MAX_VALUE? - throw new AggregationExecutionException( + // TODO: We should instrument this error. While it is correct for it to be a 400 class IllegalArgumentException, there is not + // much the user can do about that. If this occurs with any frequency, we should do something about it. + throw new IllegalArgumentException( "Can't collect more than [" + Integer.MAX_VALUE + "] buckets but attempted [" + totalOrdsToCollect + "]" ); } @@ -362,16 +364,8 @@ protected final InternalAggregation[] buildAggregationsForVariableBuckets( LongKeyedBucketOrds.BucketOrdsEnum ordsEnum = bucketOrds.ordsEnum(owningBucketOrds[ordIdx]); while (ordsEnum.next()) { if (bucketOrdsToCollect[b] != ordsEnum.ord()) { - // Not sure what can cause this; 500 seems appropriate, maybe? - throw new AggregationExecutionException( - "Iteration order of [" - + bucketOrds - + "] changed without mutating. [" - + ordsEnum.ord() - + "] should have been [" - + bucketOrdsToCollect[b] - + "]" - ); + // If we hit this, something has gone horribly wrong and we need to investigate + throw AggregationErrors.iterationOrderChangedWithoutMutating(bucketOrds.toString(), ordsEnum.ord(), bucketOrdsToCollect[b]); } buckets.add(bucketBuilder.build(ordsEnum.value(), bucketDocCount(ordsEnum.ord()), subAggregationResults[b++])); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/global/GlobalAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/global/GlobalAggregatorFactory.java index d19e60d051d08..731857575af4d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/global/GlobalAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/global/GlobalAggregatorFactory.java @@ -47,7 +47,7 @@ public Aggregator createInternal(Aggregator parent, CardinalityUpperBound cardin ); } if (cardinality != CardinalityUpperBound.ONE) { - // Not sure what would cause us to land here + // Hitting this exception is a programmer error. Hopefully never seen in production. throw new AggregationExecutionException("Aggregation [" + name() + "] must have cardinality 1 but was [" + cardinality + "]"); } return new GlobalAggregator(name, factories, context, metadata); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/prefix/IpPrefixAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/prefix/IpPrefixAggregator.java index 42b679d5e9d5f..17805d3fd835f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/prefix/IpPrefixAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/prefix/IpPrefixAggregator.java @@ -12,6 +12,7 @@ import org.apache.lucene.util.CollectionUtil; import org.elasticsearch.core.Releasables; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; +import org.elasticsearch.search.aggregations.AggregationErrors; import org.elasticsearch.search.aggregations.AggregationExecutionContext; import org.elasticsearch.search.aggregations.AggregationExecutionException; import org.elasticsearch.search.aggregations.Aggregator; @@ -195,16 +196,7 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I while (ordsEnum.next()) { long ordinal = ordsEnum.ord(); if (bucketOrdsToCollect[b] != ordinal) { - // Not sure what can cause this; 500 seems appropriate, maybe? - throw new AggregationExecutionException( - "Iteration order of [" - + bucketOrds - + "] changed without mutating. [" - + ordinal - + "] should have been [" - + bucketOrdsToCollect[b] - + "]" - ); + throw AggregationErrors.iterationOrderChangedWithoutMutating(bucketOrds.toString(), ordinal, bucketOrdsToCollect[b]); } BytesRef ipAddress = new BytesRef(); ordsEnum.readValue(ipAddress); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java index eabb65652061c..317c66654a203 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java @@ -98,7 +98,7 @@ public GlobalOrdinalsStringTermsAggregator( } else { this.collectionStrategy = cardinality.map(estimate -> { if (estimate > 1) { - // Seems like it should be 500; something went wrong if we got here + // This is a 500 class error, because we should never be able to reach it. throw new AggregationExecutionException("Dense ords don't know how to collect from many buckets"); } return new DenseGlobalOrds(); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregator.java index 5a6fbf76d9c2d..d3f0cdcfc7e0b 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/pipeline/BucketMetricsPipelineAggregator.java @@ -56,7 +56,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation Aggregation currentAgg = aggregation; while (currElement < parsedPath.size() - 1) { if (currentAgg == null) { - throw new IllegalArgumentException( + throw new AggregationExecutionException.InvalidBucketPath( "bucket_path [" + bucketsPaths()[0] + "] expected aggregation with name [" @@ -74,7 +74,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation ); if (bucket == null) { // seems not retryable, and therefore should be 400? - throw new AggregationExecutionException( + throw new AggregationExecutionException.InvalidBucketPath( "missing bucket [" + parsedPath.get(currElement).key() + "] for agg [" @@ -93,7 +93,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation currentAgg = bucket.getAggregations().get(parsedPath.get(++currElement).name()); } else { // Seems not retryable, should be 400 - throw new AggregationExecutionException( + throw new AggregationExecutionException.InvalidBucketPath( "bucket_path [" + bucketsPaths()[0] + "] indicates bucket_key [" @@ -114,7 +114,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation String msg = currentAgg == null ? "did not find multi-bucket aggregation for extraction." : "did not find multi-bucket aggregation for extraction. Found [" + currentAgg.getName() + "]"; - throw new AggregationExecutionException(msg); + throw new AggregationExecutionException.InvalidBucketPath(msg); } List sublistedPath = AggregationPath.pathElementsAsStringList(parsedPath.subList(currElement, parsedPath.size())); // First element is the current agg, so we want the rest of the path diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregator.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregator.java index de42e5307d5eb..a5915e0db16fd 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregator.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregator.java @@ -8,7 +8,6 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationErrors; -import org.elasticsearch.search.aggregations.AggregationExecutionException; import org.elasticsearch.search.aggregations.AggregationReduceContext; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.InternalAggregations; @@ -92,7 +91,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue( if (propertyValue == null) { throw AggregationErrors.incompatibleAggregationType( AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName(), - "null", + "cardinality", "null", currentAggName ); } @@ -103,7 +102,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue( throw AggregationErrors.incompatibleAggregationType( AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName(), - propertyValue.getClass().getSimpleName(), + "cardinality", propertyValue.getClass().getSimpleName(), currentAggName ); }