Creation of s3-sink plugin #2324

deepaksahu562 · 2023-02-28T15:05:57Z

Description

Created "s3-sink" plugin.
Configurations for the bucket name, key path and key pattern.
The key pattern support timestamps such as logs-${YYYY.mm}-${uniqueId}.
Collection of objects from Buffer and store it in RAM/Local file.

Issues Resolved

Github issue: #1048

Check List

New functionality s3-sink plugin.
New functionality has been documented.
- 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.

Description Created "s3-sink" plugin. Github issue : opensearch-project#1048 Added Functionality Configurations for the bucket name, key path and key pattern. The key pattern support timestamps such as logs-${YYYY.mm}-${uniqueId}. Collection of objects from Buffer and store it in RAM/Local file. Check List New functionality s3-sink plugin. New functionality has been documented. New functionality has javadoc added. Commits are signed per the DCO using --signoff 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 https://github.com/opensearch-project/data-prepper/blob/main/CONTRIBUTING.md

Configurations for the bucket name, key path and key pattern. The key pattern support timestamps such as logs-${YYYY.mm}-${uniqueId}. Collection of objects from Buffer and store it in RAM/Local file. Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Signed-off-by: deepaksahu562 <deepak.sahu562@gmail.com>

dlvenable

Hey @deepaksahu562 , Thank you for the contribution! I left some feedback on the user configuration. Since this is a draft I didn't look too much at the implementation yet.

dlvenable · 2023-03-03T02:40:21Z

...ain/java/org/opensearch/dataprepper/plugins/sink/configuration/AwsAuthenticationOptions.java

+import software.amazon.awssdk.services.sts.model.AssumeRoleRequest;
+
+/**
+    An implementation class AWS Authentication configuration


nit: Javadocs should have an * on each line. They should all be in the same column.

Thanks, David for your review. Replaced with Javadocs comment.

dlvenable · 2023-03-03T02:41:18Z

...nk/src/main/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdOptions.java

+ */
+public class ThresholdOptions {
+    static final int DEFAULT_EVENT_COUNT = 200;
+    private static final long DEFAULT_BYTE_CAPACITY = 2500;


This value seems rather low.

Thanks, David for your review. Modified with higher values.

dlvenable · 2023-03-03T02:43:53Z

...nk/src/main/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdOptions.java

+
+    @JsonProperty("byte_capacity")
+    @NotNull
+    private long byteCapacity = DEFAULT_BYTE_CAPACITY;


Data Prepper has a built-in data type for byte counts which could help here.

https://github.com/opensearch-project/data-prepper/blob/main/data-prepper-api/src/main/java/org/opensearch/dataprepper/model/types/ByteCount.java

With this, the user could configure the byte_capacity with any of the following:

byte_capacity: 512

byte_capacity: 512kb

byte_capacity: 50mb

Thanks, David for your review. Incorporated biild-in ByteCount.

dlvenable · 2023-03-03T02:45:35Z

...nk/src/main/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdOptions.java

+    private long byteCapacity = DEFAULT_BYTE_CAPACITY;
+
+    @JsonProperty("event_collection_duration")
+    private long eventCollectionDuration = DEFAULT_TIMEOUT;


Please use a Duration here. Data Prepper has a few strings that plugin authors could provide if you specify Duration as the class.

event_collection_duration: 20s

or

event_collection_duration: 5050ms

or

event_collection_duration: PT2M

Thanks, David for your review. Incorporated Duration class.

dlvenable · 2023-03-03T02:46:00Z

...nk/src/main/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdOptions.java

+    /*
+       Read the event count configuration
+    */
+    public int getEeventCount() {


Typo: getEeventCount

Thanks, David for your review.
Addressed. Please review.

dlvenable · 2023-03-03T02:48:29Z

data-prepper-plugins/s3-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/S3Sink.java

+		public void run() {
+			try {
+				while (!S3Sink.isStopRequested()) {
+					if (s3SinkConfig.getTemporaryStorage().equalsIgnoreCase(IN_MEMORY)) {


The configuration values will not change while running. So you do not need to run this check on each iteration. You can move this code to part of the initialization.

Thanks, David for your review. Moved this code to the constructor.

dlvenable · 2023-03-03T02:48:44Z

data-prepper-plugins/s3-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/S3Sink.java

+					accumulator.doAccumulate();
+				}
+			} catch (Exception e) {
+				e.printStackTrace();


Please do not call e.printStackTrace().

Thanks, David for your review. Replaced with "LOG.error("Exception while running S3SinkWorkerRunner : ", e);"

dlvenable · 2023-03-03T02:49:44Z

data-prepper-plugins/s3-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/S3Sink.java

+				}
+			} catch (Exception e) {
+				e.printStackTrace();
+				LOG.error("Exception in S3Sink : \n Error message {} \n Exception cause {}", e.getMessage(),


I wonder if we should have newlines in the logs. I'm not sure if it is very common, except in the stack trace. I'm not against it, but more wondering if it is a good pattern here.

Thanks, David for your review. Removed newlines from logs.

dlvenable · 2023-03-03T02:56:55Z

...pper-plugins/s3-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/S3SinkConfig.java

+ */
+public class S3SinkConfig {
+
+    static final String DEFAULT_BUCKET_NAME = "dataprepper";


The S3 bucket should not have a default. Only one AWS user in the entire world can have a bucket named dataprepper.

Thanks, David for your review. Addressed.

dlvenable · 2023-03-03T02:57:35Z

...pper-plugins/s3-sink/src/main/java/org/opensearch/dataprepper/plugins/sink/S3SinkConfig.java

+public class S3SinkConfig {
+
+    static final String DEFAULT_BUCKET_NAME = "dataprepper";
+    static final String DEFAULT_PATH_PREFIX = "logdata";


I'm not sure there is much value in giving a default value for the key prefix. If not specified, it can go into the root of the bucket.

Thanks, David for your review. Incorporated the changes.

…ensearch-project#1048 Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

deepaksahu562 · 2023-03-03T15:23:23Z

@dlvenable , Thanks for your review and comments.
I have incorporated all the review comments and merged them into the same PR.
Please review and provide suggestions if any.

dlvenable · 2023-03-30T16:18:30Z

...nk/src/main/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdOptions.java

+    private String byteCapacity = DEFAULT_BYTE_CAPACITY;
+
+    @JsonProperty("event_collection_duration")
+    private String eventCollectionDuration = DEFAULT_TIMEOUT;


Please change this type to a Duration. Once you do that, Data Prepper core will parse this using a standard approach. You will then be able to remove the code you have here for parsing it.

dlvenable · 2023-03-30T16:18:54Z

...nk/src/main/java/org/opensearch/dataprepper/plugins/sink/configuration/ThresholdOptions.java

+     */
+    public Duration getEventCollectionDuration() {
+
+        Duration duration;


If you make the change I suggested above, you can change this whole method to:

return eventCollectionDuration;

deepaksahu562 · 2023-04-04T16:43:46Z

Draft version, Hence closing it.

DE20436406 and others added 5 commits February 28, 2023 19:56

Created "s3-sink" plugin.

289b625

Configurations for the bucket name, key path and key pattern. The key pattern support timestamps such as logs-${YYYY.mm}-${uniqueId}. Collection of objects from Buffer and store it in RAM/Local file. Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Json sink codec

2e7d91f

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Merge branch 'opensearch-project:main' into development

61fef5f

Re-factor the s3-sink code & Apply json codec.

7ff5743

Signed-off-by: deepaksahu562 <deepak.sahu562@gmail.com>

dlvenable requested changes Mar 3, 2023

View reviewed changes

Merge branch 'opensearch-project:main' into development

bc7b3b0

deepaksahu562 added a commit to deepaksahu562/data-prepper that referenced this pull request Mar 3, 2023

Addressed review comments on PR opensearch-project#2324, Git-issue op…

2c6847e

…ensearch-project#1048 Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

Addressed review comments on PR#2324, Git-issue#1048

1077fad

Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>

dlvenable requested changes Apr 4, 2023

View reviewed changes

deepaksahu562 closed this Apr 4, 2023

deepaksahu562 deleted the development branch April 10, 2023 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creation of s3-sink plugin #2324

Creation of s3-sink plugin #2324

deepaksahu562 commented Feb 28, 2023

dlvenable left a comment

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

dlvenable Mar 3, 2023

deepaksahu562 Mar 3, 2023

deepaksahu562 commented Mar 3, 2023

dlvenable Mar 30, 2023

dlvenable Mar 30, 2023

deepaksahu562 commented Apr 4, 2023

Creation of s3-sink plugin #2324

Creation of s3-sink plugin #2324

Conversation

deepaksahu562 commented Feb 28, 2023

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

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

deepaksahu562 commented Mar 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepaksahu562 commented Apr 4, 2023