From 575627a82f1cf48eb4a51d0341c8de4f91c03a85 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 5 Oct 2023 16:06:09 -0400 Subject: [PATCH] notes for discussion on some of the remaining AggregationExecutionExceptions --- .../elasticsearch/search/aggregations/AdaptingAggregator.java | 1 + .../elasticsearch/search/aggregations/AggregationPhase.java | 1 + .../org/elasticsearch/search/aggregations/InternalOrder.java | 1 + .../search/aggregations/bucket/BucketsAggregator.java | 2 ++ .../aggregations/bucket/global/GlobalAggregatorFactory.java | 1 + .../search/aggregations/bucket/prefix/IpPrefixAggregator.java | 1 + .../bucket/terms/GlobalOrdinalsStringTermsAggregator.java | 1 + .../pipeline/BucketMetricsPipelineAggregator.java | 4 ++++ .../search/aggregations/support/ValuesSourceRegistry.java | 2 ++ .../CumulativeCardinalityPipelineAggregator.java | 2 ++ .../xpack/analytics/multiterms/InternalMultiTerms.java | 1 + .../search/aggregations/support/GeoLineMultiValuesSource.java | 1 + 12 files changed, 18 insertions(+) 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 0be4e7f729bbf..6888fbeef9510 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AdaptingAggregator.java @@ -112,6 +112,7 @@ 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/AggregationPhase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java index ce810507f3082..429053dd4dc18 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregationPhase.java @@ -64,6 +64,7 @@ private static void executeInSortOrder(SearchContext context, BucketCollector co try { searcher.search(context.rewrittenQuery(), collector); } catch (IOException e) { + // Seems like this should be 400 (non-retryable), but we clearly intentionally throw a 500 here. Why? throw new AggregationExecutionException("Could not perform time series aggregation", e); } } 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 f167d15473b0b..6f7f0c6659606 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java @@ -65,6 +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); } } 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 e2aca8c654b23..caa82ee9bf5df 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 @@ -340,6 +340,7 @@ 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( "Can't collect more than [" + Integer.MAX_VALUE + "] buckets but attempted [" + totalOrdsToCollect + "]" ); @@ -361,6 +362,7 @@ 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 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 2d63d3727c3e7..d19e60d051d08 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,6 +47,7 @@ public Aggregator createInternal(Aggregator parent, CardinalityUpperBound cardin ); } if (cardinality != CardinalityUpperBound.ONE) { + // Not sure what would cause us to land here 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 051b72092554c..42b679d5e9d5f 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 @@ -195,6 +195,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 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 8948fc64ca411..eabb65652061c 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,6 +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 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 c04203aabdfec..5a6fbf76d9c2d 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 @@ -73,6 +73,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation parsedPath.get(currElement).key() ); if (bucket == null) { + // seems not retryable, and therefore should be 400? throw new AggregationExecutionException( "missing bucket [" + parsedPath.get(currElement).key() @@ -84,12 +85,14 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation ); } if (currElement == parsedPath.size() - 1) { + // Seems not retryable, should be 400 throw new AggregationExecutionException( "invalid bucket path ends at [" + parsedPath.get(currElement).key() + "]" ); } currentAgg = bucket.getAggregations().get(parsedPath.get(++currElement).name()); } else { + // Seems not retryable, should be 400 throw new AggregationExecutionException( "bucket_path [" + bucketsPaths()[0] @@ -107,6 +110,7 @@ public final InternalAggregation doReduce(Aggregations aggregations, Aggregation } } if (currentAgg instanceof InternalMultiBucketAggregation == false) { + // Seems not retryable, should be 400 String msg = currentAgg == null ? "did not find multi-bucket aggregation for extraction." : "did not find multi-bucket aggregation for extraction. Found [" + currentAgg.getName() + "]"; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java index 827ed32aff09a..d62bfe012de7a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java @@ -183,6 +183,8 @@ public T getAggregator(RegistryKey registryKey, ValuesSourceConfig values } return supplier; } + // This should be a startup error. Should never happen, probably indicates a bad plugin if it does. Should probably log and have + // actual docs on how to resolve. throw new AggregationExecutionException( "Unregistered Aggregation [" + (registryKey != null ? registryKey.getName() : "unknown aggregation") + "]" ); 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 db9e10c167209..24a82ff01a566 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 @@ -81,6 +81,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue( List aggPathsList = AggregationPath.parse(aggPath).getPathElementsAsStringList(); Object propertyValue = bucket.getProperty(agg.getName(), aggPathsList); if (propertyValue == null) { + // Seems like a user error, change to 400 throw new AggregationExecutionException( AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() + " must reference a cardinality aggregation" ); @@ -98,6 +99,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue( } throw new AggregationExecutionException( + // seems like a user error, change to 400 AbstractPipelineAggregationBuilder.BUCKETS_PATH_FIELD.getPreferredName() + " must reference a cardinality aggregation, got: [" + propertyValue.getClass().getSimpleName() diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/InternalMultiTerms.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/InternalMultiTerms.java index ea864ddd761f7..8ede57311fb81 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/InternalMultiTerms.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/InternalMultiTerms.java @@ -194,6 +194,7 @@ static class TermsComparator implements Comparator> { @Override public int compare(List thisTerms, List otherTerms) { if (thisTerms.size() != otherTerms.size()) { + // Not clear on how this can happen. throw new AggregationExecutionException( "Merging/Reducing the multi_term aggregations failed due to different term list" + " sizes" ); diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/support/GeoLineMultiValuesSource.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/support/GeoLineMultiValuesSource.java index 3a2ebaa5761af..2be7b93928ea2 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/support/GeoLineMultiValuesSource.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/support/GeoLineMultiValuesSource.java @@ -25,6 +25,7 @@ public GeoLineMultiValuesSource(Map valuesSourceConf for (Map.Entry entry : valuesSourceConfigs.entrySet()) { final ValuesSource valuesSource = entry.getValue().getValuesSource(); if (valuesSource instanceof ValuesSource.Numeric == false && valuesSource instanceof ValuesSource.GeoPoint == false) { + // This will not resolve on retry, should be 400 throw new AggregationExecutionException( "ValuesSource type " + valuesSource.toString() + "is not supported for multi-valued aggregation" );