Added support for DynamoDBTimeWindowEvent, KinesisTimeWindowEvent and StreamsEventResponse (for KinesisEvent). #1559

ashishdhingra · 2023-08-01T22:56:16Z

Issue #, if available:
#1538
#1553

Description of changes:

Tumbling Window Events for Kinesis & Dynamo #1538 (commit 78a42a0): Added support for DynamoDBTimeWindowEvent and KinesisTimeWindowEvent.
- Documentation at https://docs.aws.amazon.com/lambda/latest/dg/with-kinesis.html#services-kinesis-windows has a note For Java functions, we recommend using a Map<String, String> to represent the state.. So in the code change, Dictionary<string, string> type was used. However, Newtonsoft Json deserializer throws error while converting int/long value to string key value. Hence, new JsonConverter was also added in Amazon.Lambda.Serialization.SystemTextJson (googled around for logic submitted by community).
Partial batch failure support with Amazon.Lambda.KinesisEvents #1553 (commit b1c92b9): Added new StreamsEventResponse class for reporting batch item failures when processing streams for KinesisEvent.

Reference:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Libraries/src/Amazon.Lambda.DynamoDBEvents/DynamoDBTimeWindowEvent.cs

Libraries/src/Amazon.Lambda.DynamoDBEvents/DynamoDBTimeWindowResponse.cs

Libraries/src/Amazon.Lambda.DynamoDBEvents/README.md

Libraries/src/Amazon.Lambda.KinesisEvents/KinesisTimeWindowEvent.cs

Libraries/src/Amazon.Lambda.KinesisEvents/KinesisTimeWindowResponse.cs

Libraries/src/Amazon.Lambda.KinesisEvents/README.md

Libraries/src/Amazon.Lambda.KinesisEvents/StreamsEventResponse.cs

normj · 2023-08-22T20:28:00Z

...aries/src/Amazon.Lambda.Serialization.SystemTextJson/Converters/LongToStringJsonConverter.cs

+
+namespace Amazon.Lambda.Serialization.SystemTextJson.Converters
+{
+    public class LongToStringJsonConverter : JsonConverter<string>


This converter makes me nervous. It will run for every string value that is serialized. Since this is just for the State property for Kinesis we shouldn't add this to the global list of converters we run.

I suggest we:

Change the converter to use Dictionary<string, string> instead of string as the generic and then implement for a Dictionary.

Rename it to DictionaryLongToStringJsonConverter

Instead of adding the converter to the global list decorate the State attributes in the Kinesis POCOs with the JsonConverter attribute. That way this converter is only used on the scenarios where we know the special scenario exists.

Update the documentation on the Kinesis POCOs informing users that they are only compatible with the serializers using System.Text.json unless you intend to also write the equivalent for Newtonsoft.

Done for most part. I don't think we need to update the documentation (for last point) since it should work.

normj · 2023-08-24T23:41:04Z

Libraries/src/Amazon.Lambda.DynamoDBEvents/Converters/DictionaryLongToStringJsonConverter.cs

+        public override void Write(Utf8JsonWriter writer, Dictionary<string, string> value, JsonSerializerOptions options)
+        {
+            // Use the built-in serializer, because it can handle dictionaries with string keys.
+            JsonSerializer.Serialize(writer, value, options);


Wouldn't this write the value as a string but given the context of this converter shouldn't it be written as a long?

Yes, it would. I still haven’t received response from Lambda team. My hunch is that the reason there is a note to use Map<string, string> for Java is that sometimes the state might contain string values. In such case, we would not be sure on whether to convert it to long or string. That’s the reason I’m handling token type string as well in ExtractValue. Please advise.

As discussed, got response from Lambda team that State field could have values represented as strings and in some cases long.

normj · 2023-08-24T23:49:23Z

Libraries/src/Amazon.Lambda.DynamoDBEvents/DynamoDBTimeWindowResponse.cs

+    using System.Collections.Generic;
+    using System.Runtime.Serialization;
+
+#if !NETSTANDARD2_0


Instead of using the ! operator just say NETCOREAPP_3_1 like every else in our libraries. Pretty soon we will need to do a sweep across all of our libraries to change all references of NETCOREAPP_3_1 to NETCOREAPP3_1_OR_GREATER and it will be easier if we are consistent with are preprocessor usage. I don't want to do that work because I need to double check the .NET SDK version running in the build system to ensure it is new enough that it has the GREATER preprocessor symbols.

I have changed it to NETCOREAPP3_1_OR_GREATER (else it was failing test case) since test case was having this conditional compilation check. Tested locally.

normj · 2023-08-24T23:49:50Z

Libraries/src/Amazon.Lambda.DynamoDBEvents/DynamoDBTimeWindowEvent.cs

+    using System;
+    using System.Collections.Generic;
+
+#if !NETSTANDARD2_0


Instead of using the ! operator just say NETCOREAPP_3_1 like every else in our libraries. Pretty soon we will need to do a sweep across all of our libraries to change all references of NETCOREAPP_3_1 to NETCOREAPP3_1_OR_GREATER and it will be easier if we are consistent with are preprocessor usage. I don't want to do that work because I need to double check the .NET SDK version running in the build system to ensure it is new enough that it has the GREATER preprocessor symbols.

I have changed it to NETCOREAPP3_1_OR_GREATER (else it was failing test case) since test case was having this conditional compilation check. Tested locally.

normj · 2023-08-24T23:50:08Z

Libraries/src/Amazon.Lambda.DynamoDBEvents/DynamoDBTimeWindowEvent.cs

+        /// <summary>
+        /// State being built up to this invoke in the time window.
+        /// </summary>
+#if !NETSTANDARD2_0


Instead of using the ! operator just say NETCOREAPP_3_1 like every else in our libraries. Pretty soon we will need to do a sweep across all of our libraries to change all references of NETCOREAPP_3_1 to NETCOREAPP3_1_OR_GREATER and it will be easier if we are consistent with are preprocessor usage. I don't want to do that work because I need to double check the .NET SDK version running in the build system to ensure it is new enough that it has the GREATER preprocessor symbols.

I have changed it to NETCOREAPP3_1_OR_GREATER (else it was failing test case) since test case was having this conditional compilation check. Tested locally.

normj · 2023-08-24T23:51:06Z

Libraries/src/Amazon.Lambda.DynamoDBEvents/DynamoDBTimeWindowResponse.cs

+        [System.Text.Json.Serialization.JsonPropertyName("state")]
+#endif
+#if !NETSTANDARD2_0
+        [JsonConverter(typeof(DictionaryLongToStringJsonConverter))]


This can be combined with the previous NETCOREAPP_3_1 condition block.

I have changed it to NETCOREAPP3_1_OR_GREATER (else it was failing test case) since test case was having this conditional compilation check. Tested locally.

philasmar

Approved, assuming you will address Norm's comments.

…son.Serialization.JsonConverter) for non-NetStandard 2.0 target(s) for handling Kinesis and DynamoDB TimeWindowEvent State property.

…s when processing streams for KinesisEvent.

ashishdhingra requested a review from normj August 1, 2023 22:56

ashishdhingra marked this pull request as draft August 1, 2023 23:05

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch from 37d9352 to d4dbddd Compare August 2, 2023 21:07

ashishdhingra changed the title ~~Added support for KinesisTimeWindowEvent.~~ Added support for DynamoDBTimeWindowEvent and KinesisTimeWindowEvent. Aug 2, 2023

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch from d4dbddd to 9a59c02 Compare August 2, 2023 21:15

ashishdhingra marked this pull request as ready for review August 2, 2023 21:15

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch 2 times, most recently from 72d4956 to 78a42a0 Compare August 3, 2023 17:01

philasmar self-requested a review August 3, 2023 17:31

ashishdhingra changed the title ~~Added support for DynamoDBTimeWindowEvent and KinesisTimeWindowEvent.~~ Added support for DynamoDBTimeWindowEvent, KinesisTimeWindowEvent and StreamsEventResponse (for KinesisEvent). Aug 3, 2023

philasmar reviewed Aug 18, 2023

View reviewed changes

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch from b1c92b9 to 5498aa3 Compare August 18, 2023 17:57

ashishdhingra requested a review from philasmar August 18, 2023 17:58

normj requested changes Aug 22, 2023

View reviewed changes

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch from 8726a45 to 5a89c48 Compare August 24, 2023 16:50

ashishdhingra requested a review from normj August 24, 2023 21:39

normj requested changes Aug 24, 2023

View reviewed changes

philasmar approved these changes Aug 25, 2023

View reviewed changes

Added support for DynamoDBTimeWindowEvent and KinesisTimeWindowEvent.

e4a46ba

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch 2 times, most recently from fd1ed95 to c749f88 Compare August 30, 2023 21:09

ashishdhingra requested a review from normj August 30, 2023 21:14

ashishdhingra added 2 commits August 31, 2023 10:00

Implemented custom DictionaryLongToStringJsonConverter (System.Text.J…

9e0be81

…son.Serialization.JsonConverter) for non-NetStandard 2.0 target(s) for handling Kinesis and DynamoDB TimeWindowEvent State property.

Added new StreamsEventResponse class for reporting batch item failure…

2dd31a1

…s when processing streams for KinesisEvent.

ashishdhingra force-pushed the user/ashdhin/KinesisTimeWindowEvent branch from c749f88 to 2dd31a1 Compare August 31, 2023 17:02

normj approved these changes Sep 1, 2023

View reviewed changes

ashishdhingra merged commit 5737503 into dev Sep 1, 2023
3 checks passed

ashishdhingra deleted the user/ashdhin/KinesisTimeWindowEvent branch September 1, 2023 16:07

ashishdhingra mentioned this pull request Sep 5, 2023

Update RELEASE.CHANGELOG.md #1579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for DynamoDBTimeWindowEvent, KinesisTimeWindowEvent and StreamsEventResponse (for KinesisEvent). #1559

Added support for DynamoDBTimeWindowEvent, KinesisTimeWindowEvent and StreamsEventResponse (for KinesisEvent). #1559

ashishdhingra commented Aug 1, 2023 •

edited

Loading

normj Aug 22, 2023

ashishdhingra Aug 30, 2023

normj Aug 24, 2023

ashishdhingra Aug 24, 2023

ashishdhingra Aug 30, 2023

normj Aug 24, 2023

ashishdhingra Aug 30, 2023

normj Aug 24, 2023

ashishdhingra Aug 30, 2023

normj Aug 24, 2023

ashishdhingra Aug 30, 2023

normj Aug 24, 2023

ashishdhingra Aug 30, 2023

philasmar left a comment

Added support for DynamoDBTimeWindowEvent, KinesisTimeWindowEvent and StreamsEventResponse (for KinesisEvent). #1559

Added support for DynamoDBTimeWindowEvent, KinesisTimeWindowEvent and StreamsEventResponse (for KinesisEvent). #1559

Conversation

ashishdhingra commented Aug 1, 2023 • 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

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

philasmar left a comment

Choose a reason for hiding this comment

ashishdhingra commented Aug 1, 2023 •

edited

Loading