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

Don't use exhausted nextToken #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martynsmith
Copy link

It seems the AWS API changed so that if you specify a nextToken that you've already used, you don't actually get new results (and a new nextToken).

This patch simply drops the nextToken from the request after it's been used (which appears to fix the --watch behaviour at least for me).

While I haven't completely read the existing bugs and pull requests for this, this seems like a relatively minor tweak to make the watch behaviour work again. (related are #74 and #62)

It seems the AWS API changed so that if you specify a nextToken that you've already used, you don't actually get new results (and a new nextToken).

This patch simply drops the nextToken from the request after it's been used (which appears to fix the `--watch` behaviour at least for me).
@coveralls
Copy link

coveralls commented Jul 18, 2017

Coverage Status

Coverage increased (+0.03%) to 91.429% when pulling d8ff2c9 on martynsmith:master into 0bc5fe3 on jorgebastida:master.

@jorgebastida
Copy link
Owner

Thanks @martynsmith for the patch. I understand the problem you are explaining... but I'm not sure removing the next_token will solve the issue. My understanding is that the following request after removing the token, aws will start over sending events from the beginning.

I wonder if the correct solution would involve keeping track of the latest timestamp of the response events, and once one of the response comes empty, remove the next_token and use the latest timestamp as the starting point.

@martynsmith
Copy link
Author

martynsmith commented Jul 29, 2017 via email

@gavrie
Copy link

gavrie commented Aug 24, 2017

@jorgebastida I've implemented a fix according to your suggestion, and it seems to address the issue quite well. If there's interest I'll prepare a PR.

@jorgebastida
Copy link
Owner

@gavrie Yes please!

@blueyed
Copy link
Contributor

blueyed commented Dec 2, 2018

I think #196 also fixes what this PR here provides/does.

@blueyed blueyed mentioned this pull request Dec 2, 2018
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.

5 participants