Adding ttl_delete parameter to metadata for DynamoDB #4982

LeeroyHannigan · 2024-09-26T10:49:13Z

Description

This change allows users to identify the source of a REMOVE event from DynamoDB streams, if it was a manual delete or a TTL delete.

Issues Resolved

Resolves #4560

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.

Signed-off-by: Lee Hannigan <[email protected]>

graytaylor0

Thanks for making this contribution! Made some minor comments that would be nice to address but overall does look good.

graytaylor0 · 2024-09-27T15:03:55Z

.../main/java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/RecordConverter.java

     * @throws Exception Exception if failed to write to buffer.
     */
    public void addToBuffer(final AcknowledgementSet acknowledgementSet,
                            final Map<String, Object> data,
                            final Map<String, Object> keys,
                            final long eventCreationTimeMillis,
                            final long eventVersionNumber,
-                            final String eventName) throws Exception {
+                            final String eventName,
+                            final Boolean userIdentity) throws Exception {


This boolean should named according to whether it's a TTL delete. Alternatively, it may make sense to make this a String and do the boolean conversion to TTL delete in this method

graytaylor0 · 2024-09-27T15:05:45Z

.../main/java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/RecordConverter.java

@@ -102,6 +105,7 @@ public void addToBuffer(final AcknowledgementSet acknowledgementSet,
        eventMetadata.setAttribute(DDB_STREAM_EVENT_NAME_METADATA_ATTRIBUTE, eventName);
        eventMetadata.setAttribute(EVENT_NAME_BULK_ACTION_METADATA_ATTRIBUTE, mapStreamEventNameToBulkAction(eventName));
        eventMetadata.setAttribute(EVENT_VERSION_FROM_TIMESTAMP, eventVersionNumber);
+        eventMetadata.setAttribute(DDB_STREAM_EVENT_USER_IDENTITY, userIdentity);


It might make sense to only add this metadata attribute when it is a TTL delete. That way in the configuration later on for routes and things of that sort, users can just do a simple check on

getMetadata("ttl_delete") != null

rather than having to check explicitly for a true value with getMetadata("ttl_delete") == true . I am fine with it either way though.

Using a simple boolean (true/false) rather than three options (null/false/true) is easier to deal with.

If anything, I'd change this from capital Boolean which allows null to lowercase boolean which as a primitive can only be true or false.

graytaylor0 · 2024-09-27T15:07:30Z

...java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/StreamRecordConverter.java

@@ -80,6 +80,7 @@ public void writeToBuffer(final AcknowledgementSet acknowledgementSet, List<Reco
        int eventCount = 0;
        for (Record record : records) {
            final long bytes = record.dynamodb().sizeBytes();
+            final Boolean userIdentity = record.userIdentity() != null && "dynamodb.amazonaws.com".equals(record.userIdentity().principalId());


This variable should be named isTtlDelete, or the userIdentity could be passed to addToBuffer directly as a String like my comment above.

Also, would be good to have "dynamodb.amazonaws.com" as a private static variable whenever it is used.

Yes, these would be good improvements to the readability.

I moved the logic to addToBuffer but instead of passing a String, I was forced to pass the Identity as it also contains Type which was suggested for checking.

dlvenable

Thank you @LeeroyHannigan for this nice feature! I have a few comments, many also shared by @graytaylor0 .

dlvenable · 2024-09-27T15:57:31Z

.../main/java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/RecordConverter.java

@@ -102,6 +105,7 @@ public void addToBuffer(final AcknowledgementSet acknowledgementSet,
        eventMetadata.setAttribute(DDB_STREAM_EVENT_NAME_METADATA_ATTRIBUTE, eventName);
        eventMetadata.setAttribute(EVENT_NAME_BULK_ACTION_METADATA_ATTRIBUTE, mapStreamEventNameToBulkAction(eventName));
        eventMetadata.setAttribute(EVENT_VERSION_FROM_TIMESTAMP, eventVersionNumber);
+        eventMetadata.setAttribute(DDB_STREAM_EVENT_USER_IDENTITY, userIdentity);


Using a simple boolean (true/false) rather than three options (null/false/true) is easier to deal with.

If anything, I'd change this from capital Boolean which allows null to lowercase boolean which as a primitive can only be true or false.

dlvenable · 2024-09-27T15:58:03Z

...java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/StreamRecordConverter.java

@@ -80,6 +80,7 @@ public void writeToBuffer(final AcknowledgementSet acknowledgementSet, List<Reco
        int eventCount = 0;
        for (Record record : records) {
            final long bytes = record.dynamodb().sizeBytes();
+            final Boolean userIdentity = record.userIdentity() != null && "dynamodb.amazonaws.com".equals(record.userIdentity().principalId());


Yes, these would be good improvements to the readability.

dlvenable · 2024-09-27T15:58:22Z

...java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/MetadataKeyAttributes.java

@@ -21,4 +21,5 @@ public class MetadataKeyAttributes {
    static final String DDB_STREAM_EVENT_NAME_METADATA_ATTRIBUTE = "dynamodb_event_name";

    static final String EVENT_TABLE_NAME_METADATA_ATTRIBUTE = "table_name";
+    static final String DDB_STREAM_EVENT_USER_IDENTITY = "ttl_delete";


Suggested change

static final String DDB_STREAM_EVENT_USER_IDENTITY = "ttl_delete";

static final String DDB_STREAM_EVENT_IS_TTL_DELETE = "ttl_delete";

Renamed static variable

dlvenable · 2024-09-27T16:00:31Z

.../org/opensearch/dataprepper/plugins/source/dynamodb/converter/StreamRecordConverterTest.java

+        when(streamConfig.getStreamViewForRemoves()).thenReturn(StreamViewType.OLD_IMAGE);
+
+        final Identity userIdentity = Identity.builder()
+                .principalId("dynamodb.amazonaws.com")


Please include a test case where the userIdentity is not null, but the principal is something other than this known principle.

It would cover the case of && "dynamodb.amazonaws.com".equals(record.userIdentity().principalId()).

...java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/StreamRecordConverter.java

Signed-off-by: Lee Hannigan <[email protected]>

LeeroyHannigan · 2024-09-29T13:23:58Z

Thank you both for review. Incorporated both your feedback, let me know if the current version is acceptable.

Adding ttl_delete parameter to metadata for DynamoDB

f1a3598

Signed-off-by: Lee Hannigan <[email protected]>

LeeroyHannigan requested review from chenqi0805, engechas, graytaylor0, dinujoh, kkondaka, KarstenSchnitter, dlvenable and oeyh as code owners September 26, 2024 10:49

graytaylor0 previously approved these changes Sep 27, 2024

View reviewed changes

dlvenable requested changes Sep 27, 2024

View reviewed changes

...java/org/opensearch/dataprepper/plugins/source/dynamodb/converter/StreamRecordConverter.java Outdated Show resolved Hide resolved

Making small adjustments to ttl_delete based on review

e31c376

Signed-off-by: Lee Hannigan <[email protected]>

LeeroyHannigan dismissed graytaylor0’s stale review via e31c376 September 29, 2024 13:20

LeeroyHannigan requested a review from dlvenable September 29, 2024 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ttl_delete parameter to metadata for DynamoDB #4982

Adding ttl_delete parameter to metadata for DynamoDB #4982

LeeroyHannigan commented Sep 26, 2024

graytaylor0 left a comment

graytaylor0 Sep 27, 2024

graytaylor0 Sep 27, 2024

dlvenable Sep 27, 2024

graytaylor0 Sep 27, 2024

dlvenable Sep 27, 2024

LeeroyHannigan Sep 29, 2024

dlvenable left a comment

dlvenable Sep 27, 2024

dlvenable Sep 27, 2024

dlvenable Sep 27, 2024

LeeroyHannigan Sep 29, 2024

dlvenable Sep 27, 2024

LeeroyHannigan commented Sep 29, 2024

	static final String DDB_STREAM_EVENT_USER_IDENTITY = "ttl_delete";
	static final String DDB_STREAM_EVENT_IS_TTL_DELETE = "ttl_delete";

Adding ttl_delete parameter to metadata for DynamoDB #4982

Are you sure you want to change the base?

Adding ttl_delete parameter to metadata for DynamoDB #4982

Conversation

LeeroyHannigan commented Sep 26, 2024

Description

Issues Resolved

Check List

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

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

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

LeeroyHannigan commented Sep 29, 2024