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

Allow users to use ListShards to check Kinesis stream readiness #43

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

StevenMcVicker
Copy link
Contributor

This addresses #42. Adds a boolean configuration field for toggling DescribeStream and ListShards in the kenesisStreamReady method that is called in Run

This might not be an ideal solution, but just putting it out there for consideration.

@garethlewin
Copy link
Contributor

Why make this a toggle and not replace DescribeStream directly with ListShards?

@garethlewin
Copy link
Contributor

Hmm, after reading this why do you propose using ListShards to tell if the stream is ready? Is the assumption that ListShards will return ResourceInUseException if kinesis is not ready?

@connormckelvey
Copy link

@garethlewin, I worked with @StevenMcVicker on this. We chose to do a toggle just to try to keep everyone happy. Happy to remove it.

We chose ListShards mainly because it seems to have the highest allowed TPS (100 tps) of any of the APIs so it would hopefully mean running into the LimitExceededException less often.

And yes, the idea is that ResourceInUseException means the stream is not in an active state. This isn't one for one with describe stream, as kinesisStreamReady allows for an UPDATING state as well. From my investigation it looks like getting the StreamStatus is only possible through DescribeStream.

@slydon
Copy link
Contributor

slydon commented Mar 15, 2024

travis is broken, I reviewed and tested this PR in my environment

@slydon slydon merged commit 9a48088 into twitchscience:master Mar 15, 2024
adatzer pushed a commit to snowplow-devops/kinsumer that referenced this pull request Nov 8, 2024
…chscience#43)

Co-authored-by: Steven McVicker <smcvicker@smcvicker-mac2.bit9.local>
Co-authored-by: Gareth Lewin <gareth@garethlewin.com>
Co-authored-by: Sean Lydon <slydon@users.noreply.github.com>
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.

4 participants