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

storagenode/piecestore: Only limit grpc requests #3342

Merged
merged 3 commits into from Oct 23, 2019
Merged

Conversation

isaachess
Copy link
Contributor

@isaachess isaachess commented Oct 22, 2019

What: Remove connection limits for drpc requests. drpc requests will continue to add to the limit, and grpc requests will respect the limit, but drpc requests will continue regardless of what the value of the current requests is.

Why: With drpc in place we no longer need to limit request limits.

Please describe the tests:

  • Test 1: The current test checking for concurrent limits is skipped because it is flaky. I don't see it within the scope of this ticket to fix that.

Please describe the performance impact:

Code Review Checklist (to be filled out by reviewer)

  • Does the PR describe what changes are being made?
  • Does the PR describe why the changes are being made?
  • Does the code follow our style guide?
  • Does the code follow our testing guide?
  • Is the PR appropriately sized? (If it could be broken into smaller PRs it should be)
  • Does the new code have enough tests? (every PR should have tests or justification otherwise. Bug-fix PRs especially)
  • Does the new code have enough documentation that answers "how do I use it?" and "what does it do?"? (both source documentation and higher level, diagrams?)
  • Does any documentation need updating?
  • Do the database access patterns make sense?

@isaachess isaachess requested a review from a team October 22, 2019 22:34
@cla-bot cla-bot bot added the cla-signed label Oct 22, 2019
@ghost ghost requested review from simongui and VinozzZ and removed request for a team October 22, 2019 22:34
Copy link
Contributor

@zeebo zeebo left a comment

Choose a reason for hiding this comment

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

Wow, super lucky that upload had two different entry points and we only care to limit uploads. I thought this was going to be way harder and gross lol.

@isaachess
Copy link
Contributor Author

@zeebo I know right? I was prepared for it to be nasty as well, but it ended up being simple to limit one but not the other.

Copy link
Contributor

@simongui simongui left a comment

Choose a reason for hiding this comment

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

Nice! I thought this was going to be uglier as well. Good job! Thanks for renaming the variable to liveGRPCRequests so it's a bit more clear to the next random person in here 😄

// liveGRPCRequests is a limit to the number of allowed concurrent GRPC
// requests. Concurrent DRPC requests count toward the limit, but DRPC
// requests are accepted regardless of the current level.
liveGRPCRequests int32
Copy link
Member

Choose a reason for hiding this comment

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

doesn't that mean this is still just liveRequests?

@jtolio
Copy link
Member

jtolio commented Oct 23, 2019

Nice! I thought this was going to be uglier as well. Good job! Thanks for renaming the variable to liveGRPCRequests so it's a bit more clear to the next random person in here smile

but it includes live drpc requests too, right? it's a counter of all live requests, regardless

@isaachess isaachess merged commit 14c7648 into master Oct 23, 2019
@isaachess isaachess deleted the ih/concurrentlimit branch October 23, 2019 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants