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

We shouldn't send requests or responses that are larger than MAX_REQUEST_SIZE #418

Merged
merged 3 commits into from
May 30, 2014

Conversation

jvshahid
Copy link
Contributor

There's no point from rejecting a request or response on the receiving end. If we need to limit the request or response then we need to split those requests and responses.

Why do we need MAX_REQUEST_SIZE and MAX_RESPONSE_SIZE anyway ?

@jvshahid
Copy link
Contributor Author

jvshahid commented Apr 8, 2014

@pauldix brought to my attention that this is already fixed for responses. We currently split the response points in half if the message exceeds the MAX_RESPONSE_SIZE until the message is smaller than the MAX_RESPONSE_SIZE. We still need two things:

  1. Do the same thing for requests
  2. Handle Multiple series in requests and responses
  3. Use protobuf MessageSize() instead of encoding the response/request to determine the length of the serialized message
  4. Leave the checks in case there's a cluster with old and new nodes (for now)

To answer the questions mentioned in the issue description. We're limiting the size of the request and response because we're reusing the read and write buffer which has a fixed size of MAX_xxx so we don't keep reallocating memory over and over.

@jvshahid jvshahid added this to the 0.5.6 milestone Apr 8, 2014
@jvshahid jvshahid added the bug label Apr 8, 2014
@jvshahid
Copy link
Contributor Author

jvshahid commented Apr 8, 2014

one more thing that i just noticed, what happens if a response is split and the number of responses exceeds the response channel size ?

@jvshahid jvshahid modified the milestones: 0.5.7, 0.5.6 Apr 9, 2014
@pauldix pauldix modified the milestones: 0.5.9, 0.5.7, 0.5.10, 0.5.11 Apr 17, 2014
@jvshahid jvshahid modified the milestones: 0.5.12, 0.5.11, 0.5.13 Apr 25, 2014
@jvshahid jvshahid modified the milestones: 0.6.0, 0.5.13, 0.6.1, 0.6.2 May 2, 2014
@jvshahid jvshahid modified the milestones: Next release, 0.6.2 May 12, 2014
@pauldix
Copy link
Member

pauldix commented May 30, 2014

This looks beautiful. I commend you, @jvshahid.

pauldix added a commit that referenced this pull request May 30, 2014
We shouldn't send requests or responses that are larger than MAX_REQUEST_SIZE
@pauldix pauldix merged commit de0a938 into master May 30, 2014
@jvshahid jvshahid modified the milestones: 0.7.2, Next release May 30, 2014
@jvshahid jvshahid deleted the fix-418-break-large-requests branch June 3, 2014 16:00
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.

2 participants