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

Support QPS throttling in NettyPerfClient #1220

Merged
merged 2 commits into from
Jul 22, 2019

Conversation

jsjtzyy
Copy link
Contributor

@jsjtzyy jsjtzyy commented Jul 19, 2019

Added some dynamic throttling logic in NettyPerfClient to issue POST
requests at given QPS.

Added some dynamic throttling logic in NettyPerfClient to issue POST
requests at given QPS.
@jsjtzyy jsjtzyy self-assigned this Jul 19, 2019
requestCount.get(), hostSleepTime);
if (targetQPS > 0 && requestCount.get() > 0) {
long val = requestCount.get() * 100 * hostSleepTime / targetQPS;
hostSleepTime = val / 100 + (val > 100 && (val % 100 >= 50 || hostSleepTime == 1) ? 1 : 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really understand your calculation here, can you elaborate?

BTW, I think some simple function like

long rtt = 1000 / request.get() - hostSleepTime;
hostSleepTime -= rrt

might just do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

reset();
perfClientMetrics.requestRate.mark();
try {
Thread.sleep(hostToSleepTime.get(hostname));
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Since this function will be called within a nonblocking function, maybe we shouldn't do a thread.sleep here.
Some think like

Channel channel = ctx.Channel();
ctx.channel().eventLoop().schedule(new Runnable() {
    // send the request out.
}, hostToSleepTime.get(hostname), TimeUnit.MILLSECOND);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting me know this. Temporarily use current version, will take your suggestion in future PR after several tests.

@justinlin-linkedin justinlin-linkedin merged commit 379d881 into linkedin:master Jul 22, 2019
@codecov-io
Copy link

Codecov Report

Merging #1220 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1220      +/-   ##
============================================
- Coverage     69.61%   69.49%   -0.12%     
+ Complexity     5510     5503       -7     
============================================
  Files           432      432              
  Lines         33744    33790      +46     
  Branches       4290     4298       +8     
============================================
- Hits          23492    23484       -8     
- Misses         9078     9124      +46     
- Partials       1174     1182       +8
Impacted Files Coverage Δ Complexity Δ
....github.ambry/tools/perf/rest/NettyPerfClient.java 0% <0%> (ø) 0 <0> (ø) ⬇️
.../main/java/com.github.ambry.store/ScanResults.java 81.25% <0%> (-1.57%) 16% <0%> (-1%)
.../java/com.github.ambry.router/DeleteOperation.java 92.19% <0%> (-1.42%) 48% <0%> (-1%)
...va/com.github.ambry.replication/ReplicaThread.java 74.62% <0%> (-0.57%) 66% <0%> (-2%)
...ain/java/com.github.ambry.router/PutOperation.java 90.61% <0%> (-0.54%) 111% <0%> (-1%)
...in/java/com.github.ambry.store/BlobStoreStats.java 71.54% <0%> (-0.42%) 103% <0%> (-1%)
...src/main/java/com.github.ambry.commons/BlobId.java 93.18% <0%> (-0.36%) 71% <0%> (-1%)
...b.ambry.network/BlockingChannelConnectionPool.java 72.3% <0%> (+1.87%) 8% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5437b5f...88b86d8. Read the comment docs.

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.

3 participants