Migrate OpenSearchSink off of plugin Settings #5273

Galactus22625 · 2024-12-18T21:42:26Z

Description

Migrate OpenSearchSink off of plugin Settings.
Tested that I can write with a pipeline into OpenSearch Sink. Tested that writing to s3 dlq works. Integration tests pass.

Issues

This resolves #5246

Notes:
IndexConfiguration.getPipeline() would always return null since it pulled form a non existent field in pipelineSettings., meaning the pipeline field for objects in bulkRequest is always null. If pipeline field is populated bulkRequest would fail.

Check List

New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- New functionality has javadoc added
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.

…nstead of PluginSettings. Rebuild readConnectionConfiguration Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

…OpenSearchSink. Remove commented out code Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

kkondaka · 2025-01-07T17:46:01Z

...rch/src/test/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSinkTest.java

@@ -258,22 +259,6 @@ void doOutput_with_invalid_version_expression_catches_NumberFormatException_and_
        verify(dynamicDocumentVersionDroppedEvents).increment();
    }

-    @Test


Why is this removed?

Forgot to re-add when i put routing_field back. fixed

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Galactus22625 · 2025-01-08T20:40:45Z

build.gradle

I should move this to opensearch/build.gradle file

…adle Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Signed-off-by: Maxwell Brown <55033421+Galactus22625@users.noreply.github.com>

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

dlvenable · 2025-01-17T17:04:04Z

.github/workflows/gradle.yml

@@ -28,7 +28,7 @@ jobs:
      run: ./gradlew --parallel --max-workers 2 build
    - name: Upload Unit Test Results
      if: always()
-      uses: actions/upload-artifact@v3
+      uses: actions/upload-artifact@v4


These should not show up as changes in this PR since they were already applied. You may need to rebase from main to correct this.

fixed by merging main into branch

…tion-changes

san81 · 2025-01-17T23:12:45Z

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

-    this.failedBulkOperationConverter = new FailedBulkOperationConverter(pluginSetting.getPipelineName(), pluginSetting.getName(),
-        pluginSetting.getName());
+    this.failedBulkOperationConverter = new FailedBulkOperationConverter(pipeline, PLUGIN_NAME,
+        PLUGIN_NAME);


If passing plugin_name is Ok in the place of plugin_id here then I would recommend not passing it and fixing the FailedBulkOperationConverter to just take one plugin_name

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

kkondaka · 2025-01-21T22:12:04Z

...st/java/org/opensearch/dataprepper/plugins/sink/opensearch/ConnectionConfigurationTests.java

-        configurationMetadata.put("authentication", Map.of("username", TEST_USERNAME, "password", TEST_PASSWORD));
-        final PluginSetting pluginSetting = getPluginSettingByConfigurationMetadata(configurationMetadata);
-        assertThrows(IllegalStateException.class,
-                () -> ConnectionConfiguration.readConnectionConfiguration(pluginSetting));


Are these two tests not applicable anymore?

The "authentication: username: password:" fields were removed since they aren't listed in the documentation, and "username: password:" already exist separately. Also part of the goal of the migration was to not allow people to put in fields that weren't valid in the configuration and I was under the impression that these fields are not supposed to be valid. since they were removed the tests arent needed anymore

@dlvenable Are you OK with this?

I talked to david earlier and he said that we should continue to support this configuration and then disallow in osi instead. I will readd

ok, re added support

kkondaka · 2025-01-22T01:51:11Z

...rch/src/test/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSinkTest.java

-        final OpenSearchSink objectUnderTest2 = createObjectUnderTest();
-        assertThat(objectUnderTest2.getDocument(event).getPipelineField(), equalTo(Optional.of(pipelineValue)));
-    }
-


Why is this test removed?

PipelineField is always null because of this comment. So test is no longer necessary
#5273 (comment)

kkondaka · 2025-01-22T01:52:27Z

...t/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/IndexConfigurationTests.java

-        metadata.put(PIPELINE, expectedPipelineValue);
-        final PluginSetting pluginSetting = getPluginSetting(metadata);
-        final IndexConfiguration indexConfiguration = IndexConfiguration.readIndexConfig(pluginSetting);
-        assertEquals(expectedPipelineValue, indexConfiguration.getPipeline());


Why is this test removed?

pipeline value is no longer retrieved from indexConfiguration, comes from pipelineDescription now

kkondaka · 2025-01-22T01:52:41Z

...t/java/org/opensearch/dataprepper/plugins/sink/opensearch/index/IndexConfigurationTests.java

-        final IndexConfiguration indexConfiguration = IndexConfiguration.readIndexConfig(pluginSetting);
-        assertEquals(expectedRoutingFieldValue, indexConfiguration.getRoutingField());
-    }
-


Why is this test removed?

forgot to re-add when routing Field was put back. readding

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Signed-off-by: Maxwell Brown <55033421+Galactus22625@users.noreply.github.com>

chenqi0805 · 2025-02-19T20:26:08Z

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

  private final SinkContext sinkContext;
  private final ExpressionEvaluator expressionEvaluator;

  private FailedBulkOperationConverter failedBulkOperationConverter;

-  private DlqProvider dlqProvider;
+  private S3DlqProvider s3DlqProvider;


We should stick with the interface instead of implementation class here.

the problem is the dlqProvider is based on PluginSetting as well, which I think we don't want to use anymore, see OpenSearchSink line 195.

I think that would not prevent us from using interface in the attribute declaration

oh you are right, fixing.

chenqi0805 · 2025-02-19T20:56:47Z

...ava/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSinkConfigurationTests.java

-
-        OpenSearchSinkConfiguration.readESConfig(pluginSetting);
-
+    @Test(expected = NullPointerException.class)


Can we throw a more informational Exception than NPE?

Null pointer is thrown when the builder fails. We use @AssertTrue which throws a message when actions are invalid in the configuration, but I don't know how to trigger this in the test cases.

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

dlvenable

Thank you @Galactus22625 !

Galactus22625 added 30 commits December 9, 2024 16:46

restruction OpenSearchSinkConfiguration to use openSearchSinkConfig i…

8b993d0

…nstead of PluginSettings. Rebuild readConnectionConfiguration Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

rebuild readIndexConfig and associated changes

23afcbf

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

rebuild readRetryConfig. Remove most instances of PluginSetting from …

acbfb7b

…OpenSearchSink. Remove commented out code Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

save point

2c65480

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Rework OpenSearchSinkConfigurationTests.java

d1cb3fa

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Rework RetryConfigurationTests.java

40f6266

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

small changes

b47c0da

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

small changes

ce07672

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

condense many test yaml files into one

19dc50e

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Rework ConnectionConfigurationTests.java

915ce5c

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Rework IndexConfigurationTests.java.java

595b766

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

copy files over from source

d150343

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

adjust all unit tests

7b78c78

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

spotless check

55eb90b

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Integration tests to OpenSearchSinkConfig from PluginSetting

e5aad51

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

build.gradle change?

0024ef0

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

remove hosts in integration test metadata

b1d5425

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

changing enforcement for required and non required fields

c949f91

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

reset awsauthconfig to normal

53398fb

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

replace authentication field from unit tests

be265c0

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

document_id_field to document_id since deprecated

5f9b9f6

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

replace pluginSetting dependent parts of integration tests

8450dfe

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

pipeline name missing from integration tests

fcbbaf6

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

find what is null

11b803b

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

initialize pipelineDescription Mock

9378889

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

initialize pluginSetting Mock

96ea7e1

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

add pluginSetting name mock

d6aa92a

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

me when I see stack trace

c53fb65

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

revert bulk retry strat

a0af91e

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

revert reversion

1c9cd9e

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

kkondaka reviewed Jan 7, 2025

View reviewed changes

put routing field test back

2e2c586

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Galactus22625 mentioned this pull request Jan 7, 2025

Migrate existing plugins to use POJO configuration classes. #5246

Closed

2 tasks

Galactus22625 requested a review from kkondaka January 7, 2025 21:01

add license header to new files

0eeada6

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Galactus22625 commented Jan 8, 2025

View reviewed changes

Galactus22625 and others added 2 commits January 8, 2025 12:43

move build.gradle changes to data-prepper-plugins/opensearch/build.gr…

7edfe65

…adle Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Merge branch 'main' into openSearch-sink-configuration-changes

ddc7faf

Signed-off-by: Maxwell Brown <55033421+Galactus22625@users.noreply.github.com>

Galactus22625 requested a review from san81 as a code owner January 16, 2025 18:21

update actions artifacts version for integration tests

3d5d8c8

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Galactus22625 mentioned this pull request Jan 16, 2025

github actions/ artifacts v3 to v4 #5339

Merged

4 tasks

fixed new test

2c78c46

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

dlvenable reviewed Jan 17, 2025

View reviewed changes

Merge branch 'opensearch-project:main' into openSearch-sink-configura…

db8d37a

…tion-changes

san81 reviewed Jan 17, 2025

View reviewed changes

FailedbulkOperationConveter takes the same input twice

7ff3d00

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

kkondaka reviewed Jan 22, 2025

View reviewed changes

readd a routing field test

c7ac019

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Galactus22625 requested a review from srikanthjg as a code owner January 22, 2025 18:27

Galactus22625 requested a review from kkondaka January 22, 2025 18:55

Galactus22625 added 2 commits January 23, 2025 08:51

remove refernces to invalid authentication configuration

122aaba

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

we want to support authentication: username: password: config

faa5c17

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

Galactus22625 requested a review from dlvenable January 24, 2025 23:21

Merge branch 'main' into openSearch-sink-configuration-changes

e124664

Signed-off-by: Maxwell Brown <55033421+Galactus22625@users.noreply.github.com>

chenqi0805 reviewed Feb 19, 2025

View reviewed changes

use itnerface instead of implementation

e6fa0ff

Signed-off-by: Maxwell Brown <mxwelwbr@amazon.com>

chenqi0805 approved these changes Feb 20, 2025

View reviewed changes

dlvenable approved these changes Feb 20, 2025

View reviewed changes

kkondaka approved these changes Feb 24, 2025

View reviewed changes

kkondaka merged commit 91fd92e into opensearch-project:main Feb 24, 2025
67 of 68 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate OpenSearchSink off of plugin Settings #5273

Migrate OpenSearchSink off of plugin Settings #5273

Galactus22625 commented Dec 18, 2024 •

edited

Loading

kkondaka Jan 7, 2025

Galactus22625 Jan 7, 2025

Galactus22625 Jan 8, 2025

Galactus22625 Jan 8, 2025

dlvenable Jan 17, 2025

Galactus22625 Jan 17, 2025

san81 Jan 17, 2025

Galactus22625 Jan 18, 2025

kkondaka Jan 21, 2025

Galactus22625 Jan 22, 2025

kkondaka Jan 24, 2025

Galactus22625 Jan 24, 2025

Galactus22625 Jan 24, 2025

kkondaka Jan 22, 2025

Galactus22625 Jan 22, 2025

kkondaka Jan 22, 2025

Galactus22625 Jan 22, 2025

kkondaka Jan 22, 2025

Galactus22625 Jan 22, 2025

kkondaka Jan 24, 2025

chenqi0805 Feb 19, 2025

Galactus22625 Feb 19, 2025 •

edited

Loading

chenqi0805 Feb 20, 2025

Galactus22625 Feb 20, 2025

chenqi0805 Feb 19, 2025

Galactus22625 Feb 20, 2025

dlvenable left a comment


		OpenSearchSinkConfiguration.readESConfig(pluginSetting);

		@Test(expected = NullPointerException.class)

Migrate OpenSearchSink off of plugin Settings #5273

Migrate OpenSearchSink off of plugin Settings #5273

Conversation

Galactus22625 commented Dec 18, 2024 • edited Loading

Description

Issues

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

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

Galactus22625 Feb 19, 2025 • 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

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

Galactus22625 commented Dec 18, 2024 •

edited

Loading

Galactus22625 Feb 19, 2025 •

edited

Loading