PersonalizeSink: add client and configuration classes #4803

ivan-tse · 2024-08-01T23:36:44Z

Description

Add client and configuration classes for Personalize Sink

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

[ x] New functionality includes testing.
New functionality has a documentation issue. Please link to it in this PR.
- [ x] 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: Ivan Tse <tseiva@amazon.com>

dlvenable

Overall, this is looking good. I have a few suggestions and comments that should be fairly straight-forward.

dlvenable · 2024-08-02T14:16:07Z

data-prepper-plugins/personalize-sink/build.gradle

+    testImplementation testLibs.slf4j.simple
+}
+
+test {


You don't need these three lines. The root project provides this configuration.

dlvenable · 2024-08-02T14:17:40Z

...-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSink.java

+     * Initialize {@link PersonalizeSinkService}
+     */
+    private void doInitializeInternal() {
+        sinkInitialized = Boolean.TRUE;


Suggested change

sinkInitialized = Boolean.TRUE;

sinkInitialized = true;

dlvenable · 2024-08-02T14:21:51Z

...-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSink.java

+ * Implementation class of personalize-sink plugin. It is responsible for receiving the collection of
+ * {@link Event} and uploading to amazon personalize.
+ */
+@DataPrepperPlugin(name = "personalize", pluginType = Sink.class, pluginConfigurationType = PersonalizeSinkConfiguration.class)


If I'm a user looking at Data Prepper documentation, I'm not sure I'd know what "personalize" means. Maybe aws_personalize would be clearer? I'm ok keeping it as you have it as well, but want to point out the possible confusion.

agree with that. I'll change it to aws_personalize

dlvenable · 2024-08-02T14:22:00Z

...-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSink.java

+        this.personalizeSinkConfig = personalizeSinkConfig;
+        this.sinkContext = sinkContext;
+
+        sinkInitialized = Boolean.FALSE;


Suggested change

sinkInitialized = Boolean.FALSE;

sinkInitialized = false;

dlvenable · 2024-08-02T14:22:52Z

...rc/main/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSinkService.java

+     * @param records received records and add into buffer.
+     */
+    void output(Collection<Record<Event>> records) {
+        LOG.info("{} records received", records.size());


Suggested change

LOG.info("{} records received", records.size());

LOG.trace("{} records received", records.size());

This is something that you would mostly use for testing the sink itself.

yes this is placeholder until the sink gets implemented. Will change to trace

dlvenable · 2024-08-02T14:31:03Z

data-prepper-plugins/personalize-sink/build.gradle

+    implementation 'org.jetbrains.kotlin:kotlin-stdlib:1.9.22'
+    implementation libs.avro.core
+    implementation(libs.hadoop.common) {
+        exclude group: 'org.eclipse.jetty'
+        exclude group: 'org.apache.hadoop', module: 'hadoop-auth'
+        exclude group: 'org.apache.zookeeper', module: 'zookeeper'
+    }
+    implementation libs.parquet.avro
+    implementation 'software.amazon.awssdk:apache-client'
+    implementation 'org.jetbrains.kotlin:kotlin-stdlib-common:1.9.22'


Suggested change

implementation 'org.jetbrains.kotlin:kotlin-stdlib:1.9.22'

implementation libs.avro.core

implementation(libs.hadoop.common) {

exclude group: 'org.eclipse.jetty'

exclude group: 'org.apache.hadoop', module: 'hadoop-auth'

exclude group: 'org.apache.zookeeper', module: 'zookeeper'

}

implementation libs.parquet.avro

implementation 'software.amazon.awssdk:apache-client'

implementation 'org.jetbrains.kotlin:kotlin-stdlib-common:1.9.22'

I do not believe you need any of these items. Certainly you shouldn't need Avro, Parquet, and Hadoop. And unless you are using a library that requires Kotlin, you can remove that.

yes let me clean up the build.gradle file

dlvenable · 2024-08-02T14:31:55Z

...rc/main/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSinkService.java

+ * Class responsible for creating PersonalizeEventsClient object, check thresholds,
+ * get new buffer and write records into buffer.
+ */
+public class PersonalizeSinkService {


Suggested change

public class PersonalizeSinkService {

class PersonalizeSinkService {

Make this package protected since you will probably only use it in PersonalizeSink.

dlvenable · 2024-08-02T14:32:18Z

...ze-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/personalize/ClientFactory.java

+import software.amazon.awssdk.core.retry.RetryPolicy;
+import software.amazon.awssdk.services.personalizeevents.PersonalizeEventsClient;
+
+public final class ClientFactory {


Suggested change

public final class ClientFactory {

final class ClientFactory {

I think this class can also be package protected.

dlvenable · 2024-08-02T14:33:43Z

...k/src/test/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSinkTest.java

+        personalizeSink = createObjectUnderTest();
+        Assertions.assertNotNull(personalizeSink);
+        personalizeSink.doInitialize();
+        assertTrue(personalizeSink.isReady(), "personalize sink is not initialized and not ready to work");


Suggested change

assertTrue(personalizeSink.isReady(), "personalize sink is not initialized and not ready to work");

assertTrue(personalizeSink.isReady(), "Expected the personalize sink to be ready, but it is reporting it is not ready.");

I think the current message may be confusing if the test fails in the future.

agreed, will change the messaging

dlvenable · 2024-08-02T14:34:17Z

...k/src/test/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSinkTest.java

+    void test_personalize_Sink_plugin_isReady_negative() {
+        personalizeSink = createObjectUnderTest();
+        Assertions.assertNotNull(personalizeSink);
+        assertFalse(personalizeSink.isReady(), "personalize sink is initialized and ready to work");


Suggested change

assertFalse(personalizeSink.isReady(), "personalize sink is initialized and ready to work");

assertFalse(personalizeSink.isReady(), "Expected the personalize sink to report that it is not ready, but it is reporting it is ready.");

dlvenable · 2024-08-02T14:37:11Z

Thank you @ivan-tse for starting this work and making this initial contribution!

Signed-off-by: Ivan Tse <tseiva@amazon.com>

kkondaka · 2024-08-06T02:27:43Z

...-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/personalize/PersonalizeSink.java

+    @Override
+    public void doInitialize() {
+        try {
+            doInitializeInternal();


Nit: This doesn't have to be a function if the function is just setting a boolean. Do you expect this function to do something different in future? If yes, keep it like it is now. Other wise I think it is unnecessary.

I'll change this and add back in the future if necessary

kkondaka · 2024-08-06T02:44:27Z

Looks good to me. I can approve this once all tests pass. David is away this week, but I see that you have addressed all his comments.

Signed-off-by: Ivan Tse <tseiva@amazon.com>

graytaylor0

Thanks for making this PR mainly just boilerplate and configuration! Made it much easier to review

graytaylor0 · 2024-08-08T20:19:44Z

.../opensearch/dataprepper/plugins/sink/personalize/configuration/AwsAuthenticationOptions.java

+
+    @AssertTrue(message = "sts_role_arn must be an IAM Role", groups = PersonalizeAdvancedValidation.class)
+    boolean isValidStsRoleArn() {
+        final Arn arn = getArn();


I think you will want to return false here if this method throws an IllegalArgumentException. Have you tested with invalid arn format and observed the exception?

Also this role should be optional in this configuration, so adding a null check to return true in the method should be enough. The reason it is optional here is because users can configure a default role in the data-prepper-config.yaml (#4559)

yes I have tested with invalid arn format.

I didn't know about the default in data-prepper-config.yaml. I'll make this optional

Signed-off-by: Ivan Tse <tseiva@amazon.com>

dlvenable

This looks good. Thanks for this contribution and making the requested changes!

PersonalizeSink: add client and configuration classes

51c6765

Signed-off-by: Ivan Tse <tseiva@amazon.com>

ivan-tse requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, KarstenSchnitter, dlvenable and oeyh as code owners August 1, 2024 23:36

dlvenable requested changes Aug 2, 2024

View reviewed changes

Addressed review comments and cleaned up build.gradle file

b928bf0

Signed-off-by: Ivan Tse <tseiva@amazon.com>

kkondaka reviewed Aug 6, 2024

View reviewed changes

ivan-tse added 2 commits August 7, 2024 10:45

Fix checkstyle

78c3d33

Signed-off-by: Ivan Tse <tseiva@amazon.com>

Remove unnecessary try-catch

3656651

Signed-off-by: Ivan Tse <tseiva@amazon.com>

graytaylor0 reviewed Aug 8, 2024

View reviewed changes

Change AwsAuthenticationOptions to be optional

09892e0

Signed-off-by: Ivan Tse <tseiva@amazon.com>

dlvenable approved these changes Aug 12, 2024

View reviewed changes

kkondaka approved these changes Aug 13, 2024

View reviewed changes

ivan-tse requested review from graytaylor0 and Keyur-S-Patel August 13, 2024 19:11

graytaylor0 approved these changes Aug 13, 2024

View reviewed changes

dlvenable merged commit 38fe2af into opensearch-project:main Aug 14, 2024
71 of 74 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PersonalizeSink: add client and configuration classes #4803

PersonalizeSink: add client and configuration classes #4803

ivan-tse commented Aug 1, 2024

dlvenable left a comment

dlvenable Aug 2, 2024

dlvenable Aug 2, 2024

dlvenable Aug 2, 2024

ivan-tse Aug 2, 2024

dlvenable Aug 2, 2024

dlvenable Aug 2, 2024

ivan-tse Aug 2, 2024

dlvenable Aug 2, 2024

ivan-tse Aug 2, 2024

dlvenable Aug 2, 2024

ivan-tse Aug 2, 2024

dlvenable Aug 2, 2024

ivan-tse Aug 2, 2024

dlvenable Aug 2, 2024

ivan-tse Aug 2, 2024

dlvenable Aug 2, 2024

dlvenable commented Aug 2, 2024

kkondaka Aug 6, 2024

ivan-tse Aug 7, 2024

kkondaka commented Aug 6, 2024

graytaylor0 left a comment

graytaylor0 Aug 8, 2024

ivan-tse Aug 9, 2024

dlvenable left a comment

	LOG.info("{} records received", records.size());
	LOG.trace("{} records received", records.size());

	public class PersonalizeSinkService {
	class PersonalizeSinkService {

	public final class ClientFactory {
	final class ClientFactory {

	assertTrue(personalizeSink.isReady(), "personalize sink is not initialized and not ready to work");
	assertTrue(personalizeSink.isReady(), "Expected the personalize sink to be ready, but it is reporting it is not ready.");

	assertFalse(personalizeSink.isReady(), "personalize sink is initialized and ready to work");
	assertFalse(personalizeSink.isReady(), "Expected the personalize sink to report that it is not ready, but it is reporting it is ready.");

PersonalizeSink: add client and configuration classes #4803

PersonalizeSink: add client and configuration classes #4803

Conversation

ivan-tse commented Aug 1, 2024

Description

Issues Resolved

Check List

dlvenable left a 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

dlvenable commented Aug 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkondaka commented Aug 6, 2024

graytaylor0 left a 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