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

Add support for backoff in scan and query #313

Merged

Conversation

bonty
Copy link
Contributor

@bonty bonty commented Nov 8, 2018

Add backoff support in scan and query operations.
Backoff functions were implemented in #246

Copy link
Member

@pboling pboling left a comment

Choose a reason for hiding this comment

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

Looks good to me! @andrykonchin what do you think?

@andrykonchin
Copy link
Member

Looks good to me.

I see some issues with backoff in query/scan operations:

  • it works badly with record_limit and batch_size options as far as repeated requests is normal situation and backoff delaying isn't needed, it only hurts performance
  • result of scan/query may be paginated by another legal reason. If data set exceeds 1Mb it's divided on pages. In this case backoff delaying isn't appropriate as well.

I don't see any way to detect throughput starvation to delay next requests right now. It would be great to reproduce this situation and find out whether it's possible to detect a case when we need to use backoff delaying like some exception etc.

@andrykonchin
Copy link
Member

@pboling @bonty What do you think?

@bonty
Copy link
Contributor Author

bonty commented Nov 14, 2018

@pboling @andrykonchin thanks for your review.

In our situation, scan and query operation is not for production requests but for adminstrator page. So We don't want to consume throughput in administrator page. And we can accept some degree of performance degradation.

It is forced to backoff in current implement, so it's better to be optional for everyone. I'm going to reimplement it, but what do you think?

@andrykonchin
Copy link
Member

@bonty

Yeah, definitely backoff should be optional. But now it's a global config option and I'm not sure whether it should be a where/query/scan parameter.

I like the ability to change configuration locally to apply new configuration only to some block of code e.g.:

Dynamoid.config.with_config(backof: ...) do
  docs = Document.where(...)
end

The only issue with this approach is that config is a singleton and any reconfiguration will affect all the API calls in all the threads of application. So in order to isolate custom temporary configuration config could be always bound to the current thread (via Thread.local).

So if you are open to make such changes now - you are welcome. If not - we will merge this PR as is. Looking forward to your reply.

@bonty
Copy link
Contributor Author

bonty commented Nov 14, 2018

I've not made any changes yet. And I think what @andrykonchin pointed is hard to implement in a short time for me. I appreciate to merge this as is.

@andrykonchin andrykonchin merged commit 392f9f7 into Dynamoid:master Nov 14, 2018
@bonty bonty deleted the feature/support-backoff-in-scan-and-query branch September 6, 2022 04:40
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.

3 participants