Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2893933004:
Throughput: Change the min number of requests in flight required (Closed)
DescriptionThroughput: Change the min number of requests in flight required
In network quality estimator (NQE), change the minimum number of
requests in flight required before a throughput observation can be
taken. The parameter is controlled using field trial params, with
default value set to 1.
BUG=725146
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
Review-Url: https://codereview.chromium.org/2893933004
Cr-Commit-Position: refs/heads/master@{#473755}
Committed: https://chromium.googlesource.com/chromium/src/+/ff83205e7249a7b9c5a52c460162be7f14b295e8
Patch Set 1 : ps #
Total comments: 7
Patch Set 2 : ryansturm comments #
Messages
Total messages: 35 (28 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Throughput: Change the min number of requests in flight required BUG= ========== to ========== Throughput: Change the min number of requests in flight required BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
The CQ bit was unchecked by tbansal@chromium.org
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Throughput: Change the min number of requests in flight required BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Throughput: Change the min number of requests in flight required The parameter is controlled using field trial params, with default value set to 1. BUG=725146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
Description was changed from ========== Throughput: Change the min number of requests in flight required The parameter is controlled using field trial params, with default value set to 1. BUG=725146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Throughput: Change the min number of requests in flight required In network quality estimator (NQE), change the minimum number of requests in flight required before a throughput observation can be taken. The parameter is controlled using field trial params, with default value set to 1. BUG=725146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. Thanks.
I didn't look at the unittest yet because it would change if you want to do what I suggested, but my suggestion is optional depending on what else will use params. https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... File net/nqe/throughput_analyzer.cc (right): https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... net/nqe/throughput_analyzer.cc:44: : params_(params), Do you plan to use this for other metrics? If not, just pass in throughput_min_request_in_flight
https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... File net/nqe/throughput_analyzer.cc (right): https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... net/nqe/throughput_analyzer.cc:44: : params_(params), On 2017/05/22 19:45:04, RyanSturm wrote: > Do you plan to use this for other metrics? If not, just pass in > throughput_min_request_in_flight Yes, I am going to do it for other metrics too.
lgtm % nits (if you want to skip the unrelated one that's fine) https://codereview.chromium.org/2893933004/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.h (right): https://codereview.chromium.org/2893933004/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.h:63: // Returns the minimum number of requests in-flight before the throughput nit: I don't like this phrasing. I'm struggling to come up with a way to say that this is "the minimum number of requests in-flight to consider the network fully utilized WRT to throughput observations being recorded". https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... File net/nqe/throughput_analyzer.cc (right): https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... net/nqe/throughput_analyzer.cc:44: : params_(params), On 2017/05/22 19:58:01, tbansal1 wrote: > On 2017/05/22 19:45:04, RyanSturm wrote: > > Do you plan to use this for other metrics? If not, just pass in > > throughput_min_request_in_flight > > Yes, I am going to do it for other metrics too. Acknowledged. https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... net/nqe/throughput_analyzer.cc:151: if (MayBeGetThroughputObservation(&downstream_kbps)) { nit please: Change the name of this to s/MayBeGetThroughputObservation/MaybeGetThroughputObservation/ Thanks
https://codereview.chromium.org/2893933004/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.h (right): https://codereview.chromium.org/2893933004/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.h:63: // Returns the minimum number of requests in-flight before the throughput On 2017/05/22 20:14:40, RyanSturm wrote: > nit: I don't like this phrasing. I'm struggling to come up with a way to say > that this is "the minimum number of requests in-flight to consider the network > fully utilized WRT to throughput observations being recorded". Done. https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... File net/nqe/throughput_analyzer.cc (right): https://codereview.chromium.org/2893933004/diff/40001/net/nqe/throughput_anal... net/nqe/throughput_analyzer.cc:151: if (MayBeGetThroughputObservation(&downstream_kbps)) { On 2017/05/22 20:14:40, RyanSturm wrote: > nit please: Change the name of this to > s/MayBeGetThroughputObservation/MaybeGetThroughputObservation/ > > Thanks Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2893933004/#ps80001 (title: "ryansturm comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495493519376230,
"parent_rev": "dfa8b4938091b9500dc5a5ac2835b95f4e24ec04", "commit_rev":
"ff83205e7249a7b9c5a52c460162be7f14b295e8"}
Message was sent while issue was closed.
Description was changed from ========== Throughput: Change the min number of requests in flight required In network quality estimator (NQE), change the minimum number of requests in flight required before a throughput observation can be taken. The parameter is controlled using field trial params, with default value set to 1. BUG=725146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester ========== to ========== Throughput: Change the min number of requests in flight required In network quality estimator (NQE), change the minimum number of requests in flight required before a throughput observation can be taken. The parameter is controlled using field trial params, with default value set to 1. BUG=725146 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester Review-Url: https://codereview.chromium.org/2893933004 Cr-Commit-Position: refs/heads/master@{#473755} Committed: https://chromium.googlesource.com/chromium/src/+/ff83205e7249a7b9c5a52c460162... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ff83205e7249a7b9c5a52c460162... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
