Support format expressions for routing in the opensearch sink #3863
Conversation
Signed-off-by: Krishna Kondaka <[email protected]>
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
asifsmohammed,
dlvenable and
oeyh
as code owners
|
|
||
| final PluginSetting pluginSetting = generatePluginSetting(null, testIndexAlias, null); | ||
| pluginSetting.getSettings().put(IndexConfiguration.ROUTING_FIELD, "${/"+testRoutingField+"}"); | ||
| when(expressionEvaluator.isValidFormatExpression(any(String.class))).thenReturn(true); |
There was a problem hiding this comment.
Since we mock this, can you just run one test manually to make sure that
routing_field: "normalField"
does not have any issues just to make sure there is no breaking change. I think it is preferable to deprecate routing_field like we did for document_id_field, and to have a new routing_id that supports the expressions. What do you think?
There was a problem hiding this comment.
The previous test is with routing_field set to normal value. There is no need to test it manually. We can support the routing_field to support both normal field and expressions. Isn't that better than introducing routing_id and deprecating the current one?
There was a problem hiding this comment.
Yes it is but we are mocking the expression evaluator so it's really just to make sure that this gets hit (
Also the only reason to change would be naming, since routing_field implies just a single field to be used, but I guess it isn't that big of a difference
There was a problem hiding this comment.
I tend to prefer making a new property with this configuration - routing. It will be consistent with document_id and clarify that you are not supply the field only.
The OpenSearch API uses the term routing, so I recommend that over routing_id.
There was a problem hiding this comment.
My only thought about routing is that it could collide with routes, but I suppose routing_id could collide just as much
|
can we add new yaml needed for this functionality in the description ? |
Signed-off-by: Krishna Kondaka <[email protected]>
@sharraj I have added the examples in the README.md instead of adding to the PR. |
| if (routingField != null) { | ||
| LOG.warn("routing_field is deprecated in favor of routing, and support for document_field will be removed in a future major version release."); |
There was a problem hiding this comment.
copy error in document_field
| routing = event.formatString(routingField, expressionEvaluator); | ||
| routingValue = event.get(routingField, String.class); | ||
| } else if (routing != null) { | ||
| if (expressionEvaluator.isValidFormatExpression(routing)) { |
There was a problem hiding this comment.
I think this function will return true for static values such as routing: "my_value". We could change this function to be an invalid expression when there is no ${} at all, but we really have been proposing users do
routing: "${my_key}"
to access fields, and then static values are static values (think index field which can be static or format expression)
Signed-off-by: Krishna Kondaka <[email protected]>
| @@ -473,11 +475,20 @@ private SerializedJson getDocument(final Event event) { | |||
| } | |||
| } | |||
|
|
|||
| String routing = (routingField != null) ? event.get(routingField, String.class) : null; | |||
| String routingValue = null; | |||
There was a problem hiding this comment.
We should have some unit tests on this code.
| try { | ||
| routingValue = event.formatString(routing, expressionEvaluator); | ||
| } catch (final ExpressionEvaluationException | EventKeyNotFoundException e) { | ||
| LOG.error("Unable to construct routing with format {}, the document_id will be generated by OpenSearch", routing, e); |
There was a problem hiding this comment.
Copy past error for document_id here. However, the routing is different from document id a bit, as it defaults to being the same value as document_id (https://www.elastic.co/guide/en/elasticsearch/reference/current/mapping-routing-field.html). So we should change this to
LOG.error("Unable to construct routing with format {}, the routing will default to being the same as the document_id", routing, e);
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
* Support format expressions for routing in the opensearch sink Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> --------- Signed-off-by: Krishna Kondaka <[email protected]> Co-authored-by: Krishna Kondaka <[email protected]> (cherry picked from commit f60c469)
Description
Added support to allow format expressions for routing field in open search sink.
Resolves #3833
Issues Resolved
Resolves #3833
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.