Add method to check if object is generically writeable in stream #54936
Conversation
When calling scripts in metric aggregation, the returned metric state is passed along to the coordinating node to do the final reduce. However, it is possible the object could contain nested state which is unknown to StreamOutput/StreamInput. This would then result in the node crashing as exceptions are not expected in the middle of serialization. This commit adds a method to StreamOutput that can determine if an object is writeable by the stream. It uses the same logic writeGenericValue, special casing each of the supported collection types to recursively determine if each contained value is itself writeable. relates elastic#54708
rjernst
added
>bug
:Analytics/Aggregations
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
Pinging @elastic/es-core-infra (:Core/Infra/Scripting) |
|
@original-brownbear I can add some tests for the new method, but I wanted to first get initial thoughts on the approach. |
| @@ -90,6 +91,7 @@ public InternalAggregation buildAggregation(long owningBucketOrdinal) { | |||
| } else { | |||
| aggregation = aggState; | |||
| } | |||
| StreamOutput.checkWriteable(aggregation); | |||
There was a problem hiding this comment.
I wonder if we could/should combine this with org.elasticsearch.common.util.CollectionUtils#ensureNoSelfReferences(java.lang.Object, java.lang.String) somehow? It seems like technically we would want to use this in all the spots that we currently also use that method to validate script returns (maybe I'm missing some detail and we wouldn't need to do this kind of validation in other script return spots and only need the no-self-references there for some reason?). That way we'd save iterating through the object to validate twice?
There was a problem hiding this comment.
I'm not sure we need both in all cases. ensureNoSelfReferences guarantees we can do any operation on the object in question (not resulting in an infinite loop). Checking if the object is writeable is only needed when we plan to write the object to transfer to another node. For example, with scripted metric aggs, we don't every write the node local results from init or map scripts, only the output of combine here that would be transferred to the coordinating node for the final reduce.
There was a problem hiding this comment.
Ok makes sense then :) => I think the approach is fine.
|
@original-brownbear I've added tests, so this is ready for a full review. |
…stic#54936) When calling scripts in metric aggregation, the returned metric state is passed along to the coordinating node to do the final reduce. However, it is possible the object could contain nested state which is unknown to StreamOutput/StreamInput. This would then result in the node crashing as exceptions are not expected in the middle of serialization. This commit adds a method to StreamOutput that can determine if an object is writeable by the stream. It uses the same logic writeGenericValue, special casing each of the supported collection types to recursively determine if each contained value is itself writeable. relates elastic#54708
) (#55561) When calling scripts in metric aggregation, the returned metric state is passed along to the coordinating node to do the final reduce. However, it is possible the object could contain nested state which is unknown to StreamOutput/StreamInput. This would then result in the node crashing as exceptions are not expected in the middle of serialization. This commit adds a method to StreamOutput that can determine if an object is writeable by the stream. It uses the same logic writeGenericValue, special casing each of the supported collection types to recursively determine if each contained value is itself writeable. relates #54708
williamrandolph
removed
:Analytics/Aggregations
When calling scripts in metric aggregation, the returned metric state is
passed along to the coordinating node to do the final reduce. However,
it is possible the object could contain nested state which is unknown to
StreamOutput/StreamInput. This would then result in the node crashing as
exceptions are not expected in the middle of serialization.
This commit adds a method to StreamOutput that can determine if an
object is writeable by the stream. It uses the same logic
writeGenericValue, special casing each of the supported collection types
to recursively determine if each contained value is itself writeable.
relates #54708