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 throttling to snaphost and restore operations #4891

Merged
merged 1 commit into from Jan 29, 2014

Conversation

imotov
Copy link
Contributor

@imotov imotov commented Jan 26, 2014

Closes #4855

@dakrone
Copy link
Member

dakrone commented Jan 28, 2014

+1, tested this locally and it works great.

}

@Override
public int read(byte[] b) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe you can just delegate to read(byte[],int,int)

@s1monw
Copy link
Contributor

s1monw commented Jan 28, 2014

left some minor comments otherwise LGTM

public int read() throws IOException {
int b = delegate.read();
if (b > 0) {
listener.onPause(rateLimiter.pause(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant something like this:

long pause = rateLimiter.pause(1);
if (pause > 0) {
 listerner.onPause(pause);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be inconsistent with RateLimitedFSDirectory.Listener I also think that onPause(0) might be useful in certain cases. However, it should be if (b >= 0) {

Copy link
Contributor

Choose a reason for hiding this comment

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

well this one is entirely different. It's a buffered input that only calls that on a flush which is every 8kb or so. This one is called for every byte? and I disagree as well on the >= though it should only be called if really needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant b >= 0 in case of bytes, not nanoseconds. In practice, this method is never called anyway, since none of the current implementations of blob store are using it to copy indices to a repository and I really hope they never will (it will be really bad performance decision anyway) So, if you feel strongly about (theoretically) not calling pause for every byte, I will implement your solution.

@s1monw
Copy link
Contributor

s1monw commented Jan 29, 2014

LGTM

@imotov imotov merged commit 2755eec into elastic:master Jan 29, 2014
@imotov imotov deleted the issue-4855-snapshot-throttling branch May 1, 2020 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add throttling to snaphost and restore operations
4 participants