Conversation
normj
left a comment
normj
left a comment
There was a problem hiding this comment.
Looks good, just a couple comments to take care of and then we can get this shipped.
| /// The approximate arrival timestamp. | ||
| /// </value> | ||
| [DataMember(Name = "approximateArrivalTimestamp")] | ||
| public long ApproximateArrivalTimestamp { get; set; } |
There was a problem hiding this comment.
Returning longs for dates in .NET makes it hard to use. Users have to understand that it is an Epoch and figure out how to convert from Epoch to DateTime.
We should do what we did for the KinesisFirehoseEvent and rename this property to ApproximateArrivalEpoch and then provide a second read only property called ApproximateArrivalTimestamp that converts the Epoch property to DateTime. Here is the code on how we did this before.
There was a problem hiding this comment.
Wouldn't be cleaner to have the property of the dto of type DateTimeOffset and use a JsonConverter responsible for converting to/from Unix epoch?
There was a problem hiding this comment.
We try in the event POCOs to not have anything specific to Newtonsoft in case we want to swap out the serializer in the future. That is why we use the DataContracts attributes instead of Newtonsoft's attributes.
You are probably right that I should have used DateTimeOffset when creating the KinesisFirehoseEvent but changing now would be breaking and since this is basically the same property I would prefer to be consistent with that.
| /// The approximate arrival timestamp. | ||
| /// </value> | ||
| [DataMember(Name = "approximateArrivalTimestamp")] | ||
| public long ApproximateArrivalTimestamp { get; set; } |
There was a problem hiding this comment.
Same comment as before about return dates as long
| public class LambdaDeliveryRecordMetadata | ||
| { | ||
| /// <summary> | ||
| /// Gets or sets the retry hint. |
There was a problem hiding this comment.
Can we add documentation that says retry hints are the number of delivery retries. Otherwise it is a confusing property.
|
All merged and NuGet package released https://www.nuget.org/packages/Amazon.Lambda.KinesisAnalyticsEvents/. Thanks for the pull request. |
3 participants
Including Kinesis Analytics events and response models with tests.