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 relates to #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 <[email protected]>

Signed-off-by: Maxwell Brown <[email protected]>

…OpenSearchSink. Remove commented out code Signed-off-by: Maxwell Brown <[email protected]>

Signed-off-by: Maxwell Brown <[email protected]>

… metadata approach Signed-off-by: Maxwell Brown <[email protected]>

Signed-off-by: Maxwell Brown <[email protected]>

…tion-changes

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 <[email protected]>

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

build.gradle

I should move this to opensearch/build.gradle file

…adle Signed-off-by: Maxwell Brown <[email protected]>

Signed-off-by: Maxwell Brown <[email protected]>

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 <[email protected]>

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?

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 <[email protected]>

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 <[email protected]>

rebuild readIndexConfig and associated changes

23afcbf

Signed-off-by: Maxwell Brown <[email protected]>

rebuild readRetryConfig. Remove most instances of PluginSetting from …

acbfb7b

…OpenSearchSink. Remove commented out code Signed-off-by: Maxwell Brown <[email protected]>

save point

2c65480

Signed-off-by: Maxwell Brown <[email protected]>

Rework OpenSearchSinkConfigurationTests.java

d1cb3fa

Signed-off-by: Maxwell Brown <[email protected]>

Rework RetryConfigurationTests.java

40f6266

Signed-off-by: Maxwell Brown <[email protected]>

small changes

b47c0da

Signed-off-by: Maxwell Brown <[email protected]>

small changes

ce07672

Signed-off-by: Maxwell Brown <[email protected]>

condense many test yaml files into one

19dc50e

Signed-off-by: Maxwell Brown <[email protected]>

Rework ConnectionConfigurationTests.java

915ce5c

Signed-off-by: Maxwell Brown <[email protected]>

Rework IndexConfigurationTests.java.java

595b766

Signed-off-by: Maxwell Brown <[email protected]>

copy files over from source

d150343

Signed-off-by: Maxwell Brown <[email protected]>

adjust all unit tests

7b78c78

Signed-off-by: Maxwell Brown <[email protected]>

spotless check

55eb90b

Signed-off-by: Maxwell Brown <[email protected]>

Integration tests to OpenSearchSinkConfig from PluginSetting

e5aad51

Signed-off-by: Maxwell Brown <[email protected]>

build.gradle change?

0024ef0

Signed-off-by: Maxwell Brown <[email protected]>

remove hosts in integration test metadata

b1d5425

Signed-off-by: Maxwell Brown <[email protected]>

changing enforcement for required and non required fields

c949f91

Signed-off-by: Maxwell Brown <[email protected]>

reset awsauthconfig to normal

53398fb

Signed-off-by: Maxwell Brown <[email protected]>

replace authentication field from unit tests

be265c0

Signed-off-by: Maxwell Brown <[email protected]>

document_id_field to document_id since deprecated

5f9b9f6

Signed-off-by: Maxwell Brown <[email protected]>

replace pluginSetting dependent parts of integration tests

8450dfe

Signed-off-by: Maxwell Brown <[email protected]>

pipeline name missing from integration tests

fcbbaf6

Signed-off-by: Maxwell Brown <[email protected]>

find what is null

11b803b

Signed-off-by: Maxwell Brown <[email protected]>

initialize pipelineDescription Mock

9378889

Signed-off-by: Maxwell Brown <[email protected]>

initialize pluginSetting Mock

96ea7e1

Signed-off-by: Maxwell Brown <[email protected]>

add pluginSetting name mock

d6aa92a

Signed-off-by: Maxwell Brown <[email protected]>

me when I see stack trace

c53fb65

Signed-off-by: Maxwell Brown <[email protected]>

revert bulk retry strat

a0af91e

Signed-off-by: Maxwell Brown <[email protected]>

revert reversion

1c9cd9e

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 added 4 commits January 3, 2025 13:57

document id field should have testIdField as value in Integration Tests

018857c

Signed-off-by: Maxwell Brown <[email protected]>

integraton tests should pass now

b1202b0

Signed-off-by: Maxwell Brown <[email protected]>

Connection configuration switched back to using metadata

5d59aef

Signed-off-by: Maxwell Brown <[email protected]>

yaml lines not needed conncection configuraion tests switched back to…

2ab0612

… metadata approach Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 requested review from dlvenable and kkondaka January 3, 2025 23:05

Galactus22625 and others added 3 commits January 6, 2025 09:02

style fix

0f5e6f1

Signed-off-by: Maxwell Brown <[email protected]>

assert True issue

21b2b97

Signed-off-by: Maxwell Brown <[email protected]>

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

6d87001

…tion-changes

kkondaka reviewed Jan 7, 2025

View reviewed changes

put routing field test back

2e2c586

Signed-off-by: Maxwell Brown <[email protected]>

Galactus22625 mentioned this pull request Jan 7, 2025

Migrate existing plugins to use POJO configuration classes. #5246

Open

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 <[email protected]>

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 <[email protected]>

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

ddc7faf

Signed-off-by: Maxwell Brown <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

kkondaka reviewed Jan 22, 2025

View reviewed changes

readd a routing field test

c7ac019

Signed-off-by: Maxwell Brown <[email protected]>

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

remove refernces to invalid authentication configuration

122aaba

Signed-off-by: Maxwell Brown <[email protected]>

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

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

Migrate OpenSearchSink off of plugin Settings #5273

Are you sure you want to change the base?

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

Galactus22625 commented Dec 18, 2024 •

edited

Loading