-
Notifications
You must be signed in to change notification settings - Fork 190
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
Parameterize MaxRequestLength #564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please check some lines with fmt.Println
?
cmd/utils/flags.go
Outdated
@@ -500,6 +500,11 @@ var ( | |||
Usage: "Wait time the rw timer waits for message writing", | |||
Value: 15 * time.Second, | |||
} | |||
MaxRequestContentLengthFlag = cli.IntFlag{ | |||
Name: "maxRequestContentLength", | |||
Usage: "Max request content length for http, websocket and gRPC", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage: "Max request content length for http, websocket and gRPC", | |
Usage: "Max request content byte length for http, websocket and gRPC", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "content length" is fine. It will not read content as byte, but string. Indeed, "Content-length" is used as an HTTP header name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to content length in byte
. What do you think? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM without minor comments.
And check the test result also
@@ -458,6 +458,20 @@ func (c *Client) newMessage(method string, paramsIn ...interface{}) (*jsonrpcMes | |||
return &jsonrpcMessage{Version: "2.0", ID: c.nextID(), Method: method, Params: params}, nil | |||
} | |||
|
|||
func (c *Client) getMessageSize(method string, args ...interface{}) (int, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why don't you put this function in `networks/rpc/websocket_test.go"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put in client.go
because it could be used at some place less. A new RPC call or a new test code in tests
package could be expected.
Proposed changes
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)Related issues
Further comments
Went through an RPC call test in a running node.