Add support for Update/Upsert/Delete operations in OpenSearch Sink #3424

kkondaka · 2023-10-02T22:46:58Z

Description

Add support for Update/Upsert/Delete operations in OpenSearch Sink
Actions can be specified with type = Create/Index/Update/Upsert/Delete and with condition
Adds support for expressions in "action" or actions "type".

Resolves #3109

Issues Resolved

Resolves #3109

Check List

[ X] New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
[X ] Commits are signed with a real name per the DCO

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.

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable · 2023-10-04T13:41:00Z

.../dataprepper/plugins/sink/opensearch/bulk/JavaClientAccumulatingUncompressedBulkRequest.java

@@ -74,6 +76,10 @@ private long estimateBulkOperationSize(BulkOperationWrapper bulkOperation) {
        if (anyDocument == null)
            return OPERATION_OVERHEAD;

+        if (anyDocument instanceof JsonNode) {
+            return OPERATION_OVERHEAD + ((JsonNode)anyDocument).toString().length();


At this point, we are serializing the JsonNode into a string. So is there any real gain in keeping the JsonNode?

It seems we can simplify by using the string and then converting to a JsonNode when needed for the update condition.

we need getJsonNode() api to pass to the update operation. I don't think reconstructing it from Json String would be efficient. And we need toJsonString() to get the size. We can do different ways but I think we will need both these

dlvenable · 2023-10-04T13:43:52Z

...nsearch/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSink.java


+      if (StringUtils.equalsIgnoreCase(eventAction, BulkAction.CREATE.toString())) {


Let's require the exact case. Ignoring the case could lead to users errors and inconsistencies. It's also too easy for us to accidentally break support for variable cased input.

OK. It is how the existing code is. Hopefully, it won't break if I make it exact case match.

OK. If that is the existing behavior, we can keep it for now. Maybe in 3.0 we can consolidate all to use only correct case.

dlvenable · 2023-10-04T13:45:09Z

...nsearch/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSink.java

-        bulkOperation = new BulkOperation.Builder()
-                .create(createOperationBuilder.build())
-                .build();
+      String eventAction = action;


This is significant logic. Can we pull it out into its own class? It will be easier to test this way and make this method more manageable.

OK, will make it a different method.

Signed-off-by: Krishna Kondaka <[email protected]>

graytaylor0 · 2023-10-05T18:28:40Z

...nsearch/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSink.java

+    if (StringUtils.equals(action, BulkAction.UPDATE.toString()) ||
+        StringUtils.equals(action, BulkAction.UPSERT.toString())) {
+          final UpdateOperation.Builder<Object> updateOperationBuilder = (action.toLowerCase() == BulkAction.UPSERT.toString()) ?
+              new UpdateOperation.Builder<>()
+                  .index(indexName)
+                  .document(jsonNode)
+                  .upsert(jsonNode) :
+              new UpdateOperation.Builder<>()
+                  .index(indexName)
+                  .document(jsonNode);
+          docId.ifPresent(updateOperationBuilder::id);
+          routing.ifPresent(updateOperationBuilder::routing);
+          bulkOperation = new BulkOperation.Builder()
+                              .update(updateOperationBuilder.build())
+                              .build();
+          return bulkOperation;


Just curious did you do any manual testing of these 2? Just to verify UPSERT will create or update and update will updates certain fields?

I have added integration test cases. Please take a look and let me know.

graytaylor0 · 2023-10-05T18:31:00Z

...c/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/IndexConfiguration.java

        public Builder withAction(final String action) {
-            checkArgument(EnumUtils.isValidEnumIgnoreCase(BulkAction.class, action), "action must be one of the following: " +  BulkAction.values());
+            // Removed validation because action may have expresions
            this.action = action;
            return this;
        }


What will be the user experience if the action that is passed is invalid? Can we at least validate that it is either a valid expression with (

data-prepper/data-prepper-api/src/main/java/org/opensearch/dataprepper/expression/ExpressionEvaluator.java

Line 36 in d3a027a

Boolean isValidExpressionStatement(final String statement);

) or one of the enum values

I am doing it at the run time after evaluating the expression. See

if (BulkAction.fromOptionValue(eventAction) == null) { LOG.error("Unknown action {}, skipping the event", eventAction); invalidActionErrorsCounter.increment(); continue; }

The isValidExpressionStatement may not be accurate if the key field passed as /xyz is seen as valid expression but there is no xyz field. Do you think it is still a good idea to do config time validation?

/xyz, xyz, etc. is not a valid expression currently. But even if it were it's still better to have the validation. we should do config time validation to check the following

private boolean isValidAction() { return isValidExpression(action) || isValidEnum(action); }

graytaylor0 · 2023-10-05T18:53:16Z

...ntegrationTest/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSinkIT.java

+    }
+
+    @Test
+    public void testBulkActionUpsertWithActions() throws IOException, InterruptedException {


Can we add one test for upsert to verify that if document does not exist it is created? Looks like we only have a test for upsert with existing document

Signed-off-by: Krishna Kondaka <[email protected]>

graytaylor0 · 2023-10-05T22:37:02Z

...c/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/IndexConfiguration.java

+            for (final Map<String, Object> actionMap: actions) {
+                String action = (String)actionMap.get("type");
+                if (action != null) {
+                    checkArgument((EnumUtils.isValidEnumIgnoreCase(BulkAction.class, action) || JacksonEvent.isValidFormatExpressions(action, expressionEvaluator)), "action must be one of the following: " + BulkAction.values());


Thanks for adding this! Would be nice to give a more direct exception for if there is an invalid expression vs some value that is not one of the enum values. Up to you if you think this is something to add

…3424) * Add support for Update/Upsert/Delete operations in OpenSearch Sink Signed-off-by: Krishna Kondaka <[email protected]> * Fixed tests and removed unused imports Signed-off-by: Krishna Kondaka <[email protected]> * Updated documentation Signed-off-by: Krishna Kondaka <[email protected]> * Added test cases to improve code coverage Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Fixed check style errors Signed-off-by: Krishna Kondaka <[email protected]> * Added another test for upsert action without prior create action Signed-off-by: Krishna Kondaka <[email protected]> * Added check for valid action strings at config time 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 36b0b9c)

…3424) (#3446) * Add support for Update/Upsert/Delete operations in OpenSearch Sink Signed-off-by: Krishna Kondaka <[email protected]> * Fixed tests and removed unused imports Signed-off-by: Krishna Kondaka <[email protected]> * Updated documentation Signed-off-by: Krishna Kondaka <[email protected]> * Added test cases to improve code coverage Signed-off-by: Krishna Kondaka <[email protected]> * Addressed review comments Signed-off-by: Krishna Kondaka <[email protected]> * Fixed check style errors Signed-off-by: Krishna Kondaka <[email protected]> * Added another test for upsert action without prior create action Signed-off-by: Krishna Kondaka <[email protected]> * Added check for valid action strings at config time 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 36b0b9c) Co-authored-by: kkondaka <[email protected]>

Add support for Update/Upsert/Delete operations in OpenSearch Sink

d885508

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka requested review from chenqi0805, engechas, graytaylor0, dinujoh, asifsmohammed, dlvenable and oeyh as code owners October 2, 2023 22:46

Krishna Kondaka added 3 commits October 2, 2023 23:35

Fixed tests and removed unused imports

1f9fc36

Signed-off-by: Krishna Kondaka <[email protected]>

Updated documentation

37aac36

Signed-off-by: Krishna Kondaka <[email protected]>

Added test cases to improve code coverage

7260d58

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka added the backport 2.5 label Oct 3, 2023

dlvenable requested changes Oct 4, 2023

View reviewed changes

Addressed review comments

71fc541

Signed-off-by: Krishna Kondaka <[email protected]>

dlvenable previously approved these changes Oct 5, 2023

View reviewed changes

Fixed check style errors

b2e9d27

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka dismissed dlvenable’s stale review via b2e9d27 October 5, 2023 18:04

graytaylor0 reviewed Oct 5, 2023

View reviewed changes

dlvenable previously approved these changes Oct 5, 2023

View reviewed changes

graytaylor0 reviewed Oct 5, 2023

View reviewed changes

Added another test for upsert action without prior create action

e112319

Signed-off-by: Krishna Kondaka <[email protected]>

kkondaka dismissed dlvenable’s stale review via e112319 October 5, 2023 19:05

Added check for valid action strings at config time

70f66da

Signed-off-by: Krishna Kondaka <[email protected]>

graytaylor0 approved these changes Oct 5, 2023

View reviewed changes

dlvenable approved these changes Oct 6, 2023

View reviewed changes

kkondaka merged commit 36b0b9c into opensearch-project:main Oct 6, 2023
44 of 45 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 6, 2023

[Backport 2.5] Add support for Update/Upsert/Delete operations in OpenSearch Sink #3446

Merged

kkondaka deleted the issue-3109 branch May 13, 2024 05:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Update/Upsert/Delete operations in OpenSearch Sink #3424

Add support for Update/Upsert/Delete operations in OpenSearch Sink #3424

kkondaka commented Oct 2, 2023

dlvenable Oct 4, 2023

kkondaka Oct 4, 2023

dlvenable Oct 4, 2023

kkondaka Oct 4, 2023

dlvenable Oct 4, 2023

dlvenable Oct 4, 2023

kkondaka Oct 4, 2023

graytaylor0 Oct 5, 2023

kkondaka Oct 5, 2023

graytaylor0 Oct 5, 2023

kkondaka Oct 5, 2023

graytaylor0 Oct 5, 2023 •

edited

Loading

graytaylor0 Oct 5, 2023

kkondaka Oct 5, 2023

graytaylor0 Oct 5, 2023


		if (StringUtils.equalsIgnoreCase(eventAction, BulkAction.CREATE.toString())) {

Add support for Update/Upsert/Delete operations in OpenSearch Sink #3424

Add support for Update/Upsert/Delete operations in OpenSearch Sink #3424

Conversation

kkondaka commented Oct 2, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graytaylor0 Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graytaylor0 Oct 5, 2023 •

edited

Loading