Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed sequenceToken from PeriodicBatchingSinkImplementationCallback #143

Merged
merged 4 commits into from
Jul 23, 2024

Conversation

VadymLevkovskyi
Copy link
Contributor

PeriodicBatchingSinkImplementationCallback class and related tests are fixed

Code related to issue #142

Copy link
Collaborator

@wparad wparad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot more places to update if this is indeed true. See the message in the docs:

The sequence token is now ignored in PutLogEvents actions. PutLogEvents actions are always accepted and never return InvalidSequenceTokenException or DataAlreadyAcceptedException even if the sequence token is not valid. You can use parallel PutLogEvents actions on the same log stream.

Is the sequence token required by the library? What happens for the minimum supported AWSSDK.CloudWatchLogs version if the SequenceToken isn't specified?

@VadymLevkovskyi
Copy link
Contributor Author

There are a lot more places to update if this is indeed true. See the message in the docs:

The sequence token is now ignored in PutLogEvents actions. PutLogEvents actions are always accepted and never return InvalidSequenceTokenException or DataAlreadyAcceptedException even if the sequence token is not valid. You can use parallel PutLogEvents actions on the same log stream.

Is the sequence token required by the library? What happens for the minimum supported AWSSDK.CloudWatchLogs version if the SequenceToken isn't specified?

Brilliant questions, thanks for highlighting them!

Regarding removing catches for mentioned exceptions - I saw it but really wasn't sure if the sequence token is the only source throwing them. Would suggest to remove them few versions later, but don't insist.

Regarding AWSSDK.CloudWatchLogs version that introduced this change - from changelog documentation it is 3.7.454.0 (2023-01-04 19:21 UTC).

Regarding necessity of sequence token - honestly I have no other idea than stated in SDK documentation and you've already seen and mentioned it.

@wparad
Copy link
Collaborator

wparad commented Jul 23, 2024

There are a lot more places to update if this is indeed true. See the message in the docs:

The sequence token is now ignored in PutLogEvents actions. PutLogEvents actions are always accepted and never return InvalidSequenceTokenException or DataAlreadyAcceptedException even if the sequence token is not valid. You can use parallel PutLogEvents actions on the same log stream.

Is the sequence token required by the library? What happens for the minimum supported AWSSDK.CloudWatchLogs version if the SequenceToken isn't specified?

Brilliant questions, thanks for highlighting them!

Regarding removing catches for mentioned exceptions - I saw it but really wasn't sure if the sequence token is the only source throwing them. Would suggest to remove them few versions later, but don't insist.

Regarding AWSSDK.CloudWatchLogs version that introduced this change - from changelog documentation it is 3.7.454.0 (2023-01-04 19:21 UTC).

Regarding necessity of sequence token - honestly I have no other idea than stated in SDK documentation and you've already seen and mentioned it.

So, I think the only thing I want to avoid is an in-between state. Assuming we continue with this PR, we can remove those exceptions as they are directly called out by the sequenceToken warning, and from experience they are only thrown when using an invalid sequence token.

My biggest concern is that right now, we support versions of the AWSSDK.CloudWatchLogs version 3.7.102.27, right now is set to be >= 3.7.1.

My recommendation here would be:

  • Remove all the usages of sequenceToken
  • Remove all the usages of the two exceptions that now will not be thrown
  • Update the minimum required library version to 3.7.102.27
  • Update the this library's package minor version number (to 4.3, I think)

…s handling dropped from main code and tests, same for PutLogEventsResponse.NextSequenceToken and PutLogEventsRequest.SequenceToken
@VadymLevkovskyi
Copy link
Contributor Author

...

So, I think the only thing I want to avoid is an in-between state. Assuming we continue with this PR, we can remove those exceptions as they are directly called out by the sequenceToken warning, and from experience they are only thrown when using an invalid sequence token.

My biggest concern is that right now, we support versions of the AWSSDK.CloudWatchLogs version 3.7.102.27, right now is set to be >= 3.7.1.

My recommendation here would be:

  • Remove all the usages of sequenceToken
  • Remove all the usages of the two exceptions that now will not be thrown
  • Update the minimum required library version to 3.7.102.27
  • Update the this library's package minor version number (to 4.3, I think)

Done

@wparad wparad merged commit 9a5e6da into Cimpress-MCP:master Jul 23, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants