notes for discussion on some of the remaining AggregationExecutionExc…

…eptions

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);
         }
     }

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);
         }
     }

server/src/main/java/org/elasticsearch/search/aggregations/InternalOrder.java

@@ -65,6 +65,7 @@ public <T extends Bucket> Comparator<T> 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);
             }
         }

server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java

@@ -340,6 +340,7 @@ protected final <B> 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 <B> 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

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);

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

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();

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() + "]";

server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java

@@ -183,6 +183,8 @@ public <T> T getAggregator(RegistryKey<T> 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") + "]"
         );

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityPipelineAggregator.java

@@ -81,6 +81,7 @@ private AbstractHyperLogLogPlusPlus resolveBucketValue(
         List<String> 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()

x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/multiterms/InternalMultiTerms.java

@@ -194,6 +194,7 @@ static class TermsComparator implements Comparator<List<Object>> {
         @Override
         public int compare(List<Object> thisTerms, List<Object> 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"
                 );

x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/support/GeoLineMultiValuesSource.java

@@ -25,6 +25,7 @@ public GeoLineMultiValuesSource(Map<String, ValuesSourceConfig> valuesSourceConf
         for (Map.Entry<String, ValuesSourceConfig> 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"
                 );