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

Rpc limits (2x) #1394

Merged
merged 4 commits into from
Jan 10, 2020
Merged

Rpc limits (2x) #1394

merged 4 commits into from
Jan 10, 2020

Conversation

shargon
Copy link
Member

@shargon shargon commented Jan 3, 2020

Close #1393 for 2x

#TODO Trying to find the best way for add a read Timeout @Tommo-L could you help me with this?

@shargon shargon requested a review from Tommo-L January 3, 2020 08:59
vncoelho
vncoelho previously approved these changes Jan 6, 2020
@superboyiii
Copy link
Member

superboyiii commented Jan 7, 2020

I'll make test on this PR. It will take several hours.

@superboyiii
Copy link
Member

superboyiii commented Jan 7, 2020

@shargon Can we make options.Limits.MaxConcurrentConnections be got from config.json? If it's from Peer.DefaultMaxConnections (now it's 40), I think it's will be a limit of TPS which might not be a good solution. We'd better make it optional.

@superboyiii
Copy link
Member

superboyiii commented Jan 7, 2020

@shargon I've made test on MaxConcurrentConnections and set it to 2 on config.json. It seems doesn't work. I tried to make 12 thread groups in one second. And in every thread group, I made 10 users. I post these requests in one second. However, all requests returned back within correct responses.
image
image
image
By the way, Shall MaxConcurrentConnections not be defined in setting.cs?
image

@shargon
Copy link
Member Author

shargon commented Jan 7, 2020

I've made test on MaxConcurrentConnections and set it to 2 on config.json. It seems doesn't work. I tried to make 12 thread groups in one second. And in every thread group, I made 10 users. I post these requests in one second. However, all requests returned back within correct responses.

This require an extra change in neo-cli, here we only can allow to be configured, but the config of RPC is on neo-cli

@Tommo-L
Copy link
Contributor

Tommo-L commented Jan 8, 2020

This require an extra change in neo-cli, here we only can allow to be configured, but the config of RPC is on neo-cli

If so, I suggest adding MaxConcurrentConnections on Neo3, not on Neo2.x, otherwise we need to update the neo-cli.

@superboyiii
Copy link
Member

I've made test on MaxConcurrentConnections and set it to 2 on config.json. It seems doesn't work. I tried to make 12 thread groups in one second. And in every thread group, I made 10 users. I post these requests in one second. However, all requests returned back within correct responses.

This require an extra change in neo-cli, here we only can allow to be configured, but the config of RPC is on neo-cli

Yes, for this PR, I tested it and it worked as expected. But to user friendly, I think we should open an interface on config.json of node and return the authority to users. Otherwise, it's impossible to ask a user to change the code of neo and make it as a dependency of neo-cli, then publish neo-cli by themselves.

@shargon
Copy link
Member Author

shargon commented Jan 8, 2020

If so, I suggest adding MaxConcurrentConnections on Neo3, not on Neo2.x, otherwise we need to update the neo-cli.

Now it's like this, you can do that or not, but if we won't change neo-cli, we can't configure it, it will take the default value

@erikzhang
Copy link
Member

Please, don't waste your time on 2.x.

@superboyiii
Copy link
Member

superboyiii commented Jan 9, 2020

@erikzhang @shargon Shall we go on this PR or stop? Whatever, it pass the test.

@shargon
Copy link
Member Author

shargon commented Jan 9, 2020

If it pass, I think that should be merged, it's already done

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

It is a simple change, no bad trade-off.

@erikzhang erikzhang merged commit 54d0f29 into neo-project:master-2.x Jan 10, 2020
@erikzhang erikzhang deleted the rpc-limits branch January 10, 2020 05:47
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.

5 participants