Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1999303002:
Fix flaky NQE cronet test (Closed)
DescriptionFix flaky NQE cronet test
Example of a flaky run from testbot:
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cronet_tester/builds/168
and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Builder/builds/2184
A recent NQE CL changed the logic so that throughput
observations are now posted to NQE (before that CL, the
observations were available synchronously).
BUG=614227
Committed: https://crrev.com/86d66363a9aad84556298c81c4a5af935d67d03b
Cr-Commit-Position: refs/heads/master@{#396488}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : #
Total comments: 8
Patch Set 3 : Using Executor instead of TestExecutor #
Total comments: 6
Patch Set 4 : Addressed comments #Patch Set 5 : #Patch Set 6 : Add locks so that counts can be read concurrently #Messages
Total messages: 39 (19 generated)
Description was changed from ========== Fix flaky NQE cronet test BUG= ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... BUG= ==========
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + mef@chromium.org
tbansal@chromium.org changed reviewers: + mef@chromium.org
mef: ptal. Thanks.
mef: ptal. Thanks.
Description was changed from ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... BUG= ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... BUG= ==========
Description was changed from ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... BUG= ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... BUG=614227 ==========
xunjieli@chromium.org changed reviewers: + xunjieli@chromium.org
https://codereview.chromium.org/1999303002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:286: // Throughput observation is posted to network quality estimator, and running two requests The comment is a little vague, and I don't quite understand why starting a second request forces the NQE throughput callback to be flushed. When will we know the throughput observation of the first request ? Will it definitely occur before the second request?
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
tbansal@chromium.org changed reviewers: - mef@chromium.org
xunjieli: ptal. Thanks (also moved mef to CC). https://codereview.chromium.org/1999303002/diff/20001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/20001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:286: // Throughput observation is posted to network quality estimator, and running two requests On 2016/05/24 01:54:43, xunjieli wrote: > The comment is a little vague, and I don't quite understand why starting a > second request forces the NQE throughput callback to be flushed. > When will we know the throughput observation of the first request ? Will it > definitely occur before the second request? > I updated the comment. Let me know if it is still confusing.
Description was changed from ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... BUG=614227 ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before it was available synchronously). BUG=614227 ==========
Description was changed from ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before it was available synchronously). BUG=614227 ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before that CL, the observations were available synchronously). BUG=614227 ==========
https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:275: // TODO(xunjieli): Remove annotation after crbug.com/539519 is fixed. Since you are here, do you mind getting rid of this TODO and @SuppressWarnings for this test and the three tests above this one? https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:288: // UrlRequest would post before the second UrlRequest starts. I don't quite understand the threading model. The throughput observations are posted to |testExecutor| which is different from the executors used by the UrlRequests' callbacks. Do you mean that UrlRequest's throughput observation will be posted from the *network thread* to the Java executor before the second UrlRequest is started on the network thread? Is it better then to explicitly wait until throughput observation is received? Something like: ConditionalVariable waitForThroughput = new ConditionalVariable(); TestNetworkQualityListener networkQualityListener = new TestNetworkQualityListener() { @Override public void onThroughputObservation(int throughputKbps, long when, int source) { super.onThroughputObservation(throughputKps, when, source); waitForThroughput.open(); } } .... callback.blockForDone(); waitForThroughput.block(); ....
https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:288: // UrlRequest would post before the second UrlRequest starts. On 2016/05/24 18:26:39, xunjieli wrote: > I don't quite understand the threading model. The throughput observations are > posted to |testExecutor| which is different from the executors used by the > UrlRequests' callbacks. Do you mean that UrlRequest's throughput observation > will be posted from the *network thread* to the Java executor before the second > UrlRequest is started on the network thread? > > Is it better then to explicitly wait until throughput observation is received? > > Something like: > > ConditionalVariable waitForThroughput = new ConditionalVariable(); > > TestNetworkQualityListener networkQualityListener = new > TestNetworkQualityListener() { > @Override > public void onThroughputObservation(int throughputKbps, long when, int > source) { > super.onThroughputObservation(throughputKps, when, source); > waitForThroughput.open(); > } > } > > .... > > callback.blockForDone(); > waitForThroughput.block(); > .... Context on how NQE works: After the request completes, ThroughputAnalyzer posts the throughput observation to NQE on IO thread. NQE then notifies its observers. Cronet is one of the observer and when Cronet gets the notification, it posts the observation to all ThroughputListeners on NQEExecutor (including TestNetworkQualityListener). So, in the test, we need a way to wait for the first IO post to execute. As soon as that executes, we need to call testExecutor.runAllTasks(). The problem is that the post from ThroughputAnalyzer to NQE may execute after callback.blockForDone(). I am not sure how to wait in the test to make sure that the first IO post is executed.
https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:288: // UrlRequest would post before the second UrlRequest starts. On 2016/05/25 00:02:22, tbansal1 wrote: > On 2016/05/24 18:26:39, xunjieli wrote: > > I don't quite understand the threading model. The throughput observations are > > posted to |testExecutor| which is different from the executors used by the > > UrlRequests' callbacks. Do you mean that UrlRequest's throughput observation > > will be posted from the *network thread* to the Java executor before the > second > > UrlRequest is started on the network thread? > > > > Is it better then to explicitly wait until throughput observation is received? > > > > Something like: > > > > ConditionalVariable waitForThroughput = new ConditionalVariable(); > > > > TestNetworkQualityListener networkQualityListener = new > > TestNetworkQualityListener() { > > @Override > > public void onThroughputObservation(int throughputKbps, long when, int > > source) { > > super.onThroughputObservation(throughputKps, when, source); > > waitForThroughput.open(); > > } > > } > > > > .... > > > > callback.blockForDone(); > > waitForThroughput.block(); > > .... > > Context on how NQE works: After the request completes, > ThroughputAnalyzer posts the throughput observation to NQE on IO thread. NQE > then notifies its observers. Cronet is one of the observer and when Cronet gets > the notification, it posts the observation to all ThroughputListeners on > NQEExecutor (including TestNetworkQualityListener). > > So, in the test, we need a way to wait for the first IO post to execute. As soon > as that executes, we need to call testExecutor.runAllTasks(). > > The problem is that the post from ThroughputAnalyzer to NQE may execute after > callback.blockForDone(). > > I am not sure how to wait in the test to make sure that the first IO post is > executed. I thought opening the conditional variable in onThroughputObservation will make sure the IO is completed? @Override public void onThroughputObservation(int throughputKbps, long when, int source) { super.onThroughputObservation(throughputKps, when, source); // Open the conditional variable here. waitForThroughput.open(); } } callback.blockForDone(); // Wait for conditional variable to open waitForThroughput.block(); testExecutor.runAllTasks(); Does this solve the flakiness?
https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:288: // UrlRequest would post before the second UrlRequest starts. On 2016/05/25 20:37:50, xunjieli wrote: > On 2016/05/25 00:02:22, tbansal1 wrote: > > On 2016/05/24 18:26:39, xunjieli wrote: > > > I don't quite understand the threading model. The throughput observations > are > > > posted to |testExecutor| which is different from the executors used by the > > > UrlRequests' callbacks. Do you mean that UrlRequest's throughput observation > > > will be posted from the *network thread* to the Java executor before the > > second > > > UrlRequest is started on the network thread? > > > > > > Is it better then to explicitly wait until throughput observation is > received? > > > > > > Something like: > > > > > > ConditionalVariable waitForThroughput = new ConditionalVariable(); > > > > > > TestNetworkQualityListener networkQualityListener = new > > > TestNetworkQualityListener() { > > > @Override > > > public void onThroughputObservation(int throughputKbps, long when, > int > > > source) { > > > super.onThroughputObservation(throughputKps, when, source); > > > waitForThroughput.open(); > > > } > > > } > > > > > > .... > > > > > > callback.blockForDone(); > > > waitForThroughput.block(); > > > .... > > > > Context on how NQE works: After the request completes, > > ThroughputAnalyzer posts the throughput observation to NQE on IO thread. NQE > > then notifies its observers. Cronet is one of the observer and when Cronet > gets > > the notification, it posts the observation to all ThroughputListeners on > > NQEExecutor (including TestNetworkQualityListener). > > > > So, in the test, we need a way to wait for the first IO post to execute. As > soon > > as that executes, we need to call testExecutor.runAllTasks(). > > > > The problem is that the post from ThroughputAnalyzer to NQE may execute after > > callback.blockForDone(). > > > > I am not sure how to wait in the test to make sure that the first IO post is > > executed. > > I thought opening the conditional variable in onThroughputObservation will make > sure the IO is completed? You are right, it makes sure that IO is completed. That pushes the observation to Cronet. However, testExecutor (aka NQEExecutor) also needs to run so that the observation is pushed from Cronet to the TestListener. https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/... > > > @Override > public void onThroughputObservation(int throughputKbps, long when, int > source) { > super.onThroughputObservation(throughputKps, when, source); > // Open the conditional variable here. > waitForThroughput.open(); > } > } > callback.blockForDone(); > // Wait for conditional variable to open > waitForThroughput.block(); > testExecutor.runAllTasks(); > > > Does this solve the flakiness? I tried this before. This would forever block on waitForThroughput (in some cases based on race) because it is waiting for the testExecutor to post the observation from NQE to the TestListener. One other really hacky fix is (just to illustrate what actually works): callback.blockForDone(); waitForThroughput.block(5000); // Wait for 5 seconds, the IO task should be posted in the meantime. testExecutor.runAllTasks(); // This will post the observation from Cronet to test listener and open waitForThroughput. waitForThroughput.block(); // This should unblock immediately because waitForThroughput would be open.
https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:288: // UrlRequest would post before the second UrlRequest starts. On 2016/05/25 21:04:13, tbansal1 wrote: > On 2016/05/25 20:37:50, xunjieli wrote: > > On 2016/05/25 00:02:22, tbansal1 wrote: > > > On 2016/05/24 18:26:39, xunjieli wrote: > > > > I don't quite understand the threading model. The throughput observations > > are > > > > posted to |testExecutor| which is different from the executors used by the > > > > UrlRequests' callbacks. Do you mean that UrlRequest's throughput > observation > > > > will be posted from the *network thread* to the Java executor before the > > > second > > > > UrlRequest is started on the network thread? > > > > > > > > Is it better then to explicitly wait until throughput observation is > > received? > > > > > > > > Something like: > > > > > > > > ConditionalVariable waitForThroughput = new ConditionalVariable(); > > > > > > > > TestNetworkQualityListener networkQualityListener = new > > > > TestNetworkQualityListener() { > > > > @Override > > > > public void onThroughputObservation(int throughputKbps, long when, > > int > > > > source) { > > > > super.onThroughputObservation(throughputKps, when, source); > > > > waitForThroughput.open(); > > > > } > > > > } > > > > > > > > .... > > > > > > > > callback.blockForDone(); > > > > waitForThroughput.block(); > > > > .... > > > > > > Context on how NQE works: After the request completes, > > > ThroughputAnalyzer posts the throughput observation to NQE on IO thread. NQE > > > then notifies its observers. Cronet is one of the observer and when Cronet > > gets > > > the notification, it posts the observation to all ThroughputListeners on > > > NQEExecutor (including TestNetworkQualityListener). > > > > > > So, in the test, we need a way to wait for the first IO post to execute. As > > soon > > > as that executes, we need to call testExecutor.runAllTasks(). > > > > > > The problem is that the post from ThroughputAnalyzer to NQE may execute > after > > > callback.blockForDone(). > > > > > > I am not sure how to wait in the test to make sure that the first IO post is > > > executed. > > > > I thought opening the conditional variable in onThroughputObservation will > make > > sure the IO is completed? > You are right, it makes sure that IO is completed. That pushes the observation > to Cronet. However, testExecutor (aka NQEExecutor) also needs to run so that the > observation is pushed from Cronet to the TestListener. > https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/... > > > > > > @Override > > public void onThroughputObservation(int throughputKbps, long when, int > > source) { > > super.onThroughputObservation(throughputKps, when, source); > > // Open the conditional variable here. > > waitForThroughput.open(); > > } > > } > > callback.blockForDone(); > > // Wait for conditional variable to open > > waitForThroughput.block(); > > testExecutor.runAllTasks(); > > > > > > Does this solve the flakiness? > I tried this before. This would forever block on waitForThroughput (in some > cases based on race) because it is waiting for the testExecutor to post the > observation from NQE to the TestListener. > > One other really hacky fix is (just to illustrate what actually works): > callback.blockForDone(); > waitForThroughput.block(5000); // Wait for 5 seconds, the IO task should be > posted in the meantime. > testExecutor.runAllTasks(); // This will post the observation from Cronet to > test listener and open waitForThroughput. > waitForThroughput.block(); // This should unblock immediately because > waitForThroughput would be open. > Ah, I see. I didn't realize TestExecutor runs tasks on current thread(which is the test thread). Since we are blocking test thread, we will be essentially blocking TestExecutor. It should be easy to solve this. Just create a executor that runs tasks on a separate thread(you can use Executors.newSingleThreadExecutor()), and then blocking test thread will be fine. Blocking test thread will ensure the IO completes.
xunjieli: PTAL. Thanks for the help!!
https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:132: ConditionVariable mWaitForThroughput = null; nit: maybe get rid of "=null", and add "final" keyword. And move the final member above the non-final ones. https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:298: waitForThroughput.block(); Is throughtput observation always going to be posted after the RTT observation?
ptal. Thanks. https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:275: // TODO(xunjieli): Remove annotation after crbug.com/539519 is fixed. On 2016/05/24 18:26:38, xunjieli wrote: > Since you are here, do you mind getting rid of this TODO and @SuppressWarnings > for this test and the three tests above this one? Done. https://codereview.chromium.org/1999303002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:288: // UrlRequest would post before the second UrlRequest starts. On 2016/05/26 13:54:47, xunjieli wrote: > On 2016/05/25 21:04:13, tbansal1 wrote: > > On 2016/05/25 20:37:50, xunjieli wrote: > > > On 2016/05/25 00:02:22, tbansal1 wrote: > > > > On 2016/05/24 18:26:39, xunjieli wrote: > > > > > I don't quite understand the threading model. The throughput > observations > > > are > > > > > posted to |testExecutor| which is different from the executors used by > the > > > > > UrlRequests' callbacks. Do you mean that UrlRequest's throughput > > observation > > > > > will be posted from the *network thread* to the Java executor before the > > > > second > > > > > UrlRequest is started on the network thread? > > > > > > > > > > Is it better then to explicitly wait until throughput observation is > > > received? > > > > > > > > > > Something like: > > > > > > > > > > ConditionalVariable waitForThroughput = new ConditionalVariable(); > > > > > > > > > > TestNetworkQualityListener networkQualityListener = new > > > > > TestNetworkQualityListener() { > > > > > @Override > > > > > public void onThroughputObservation(int throughputKbps, long > when, > > > int > > > > > source) { > > > > > super.onThroughputObservation(throughputKps, when, source); > > > > > waitForThroughput.open(); > > > > > } > > > > > } > > > > > > > > > > .... > > > > > > > > > > callback.blockForDone(); > > > > > waitForThroughput.block(); > > > > > .... > > > > > > > > Context on how NQE works: After the request completes, > > > > ThroughputAnalyzer posts the throughput observation to NQE on IO thread. > NQE > > > > then notifies its observers. Cronet is one of the observer and when > Cronet > > > gets > > > > the notification, it posts the observation to all ThroughputListeners on > > > > NQEExecutor (including TestNetworkQualityListener). > > > > > > > > So, in the test, we need a way to wait for the first IO post to execute. > As > > > soon > > > > as that executes, we need to call testExecutor.runAllTasks(). > > > > > > > > The problem is that the post from ThroughputAnalyzer to NQE may execute > > after > > > > callback.blockForDone(). > > > > > > > > I am not sure how to wait in the test to make sure that the first IO post > is > > > > executed. > > > > > > I thought opening the conditional variable in onThroughputObservation will > > make > > > sure the IO is completed? > > You are right, it makes sure that IO is completed. That pushes the observation > > to Cronet. However, testExecutor (aka NQEExecutor) also needs to run so that > the > > observation is pushed from Cronet to the TestListener. > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/... > > > > > > > > > @Override > > > public void onThroughputObservation(int throughputKbps, long when, > int > > > source) { > > > super.onThroughputObservation(throughputKps, when, source); > > > // Open the conditional variable here. > > > waitForThroughput.open(); > > > } > > > } > > > callback.blockForDone(); > > > // Wait for conditional variable to open > > > waitForThroughput.block(); > > > testExecutor.runAllTasks(); > > > > > > > > > Does this solve the flakiness? > > I tried this before. This would forever block on waitForThroughput (in some > > cases based on race) because it is waiting for the testExecutor to post the > > observation from NQE to the TestListener. > > > > One other really hacky fix is (just to illustrate what actually works): > > callback.blockForDone(); > > waitForThroughput.block(5000); // Wait for 5 seconds, the IO task should be > > posted in the meantime. > > testExecutor.runAllTasks(); // This will post the observation from Cronet to > > test listener and open waitForThroughput. > > waitForThroughput.block(); // This should unblock immediately because > > waitForThroughput would be open. > > > > Ah, I see. I didn't realize TestExecutor runs tasks on current thread(which is > the test thread). Since we are blocking test thread, we will be essentially > blocking TestExecutor. > > It should be easy to solve this. Just create a executor that runs tasks on a > separate thread(you can use Executors.newSingleThreadExecutor()), and then > blocking test thread will be fine. Blocking test thread will ensure the IO > completes. > > Acknowledged. https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:132: ConditionVariable mWaitForThroughput = null; On 2016/05/26 17:55:30, xunjieli wrote: > nit: maybe get rid of "=null", and add "final" keyword. And move the final > member above the non-final ones. Done. https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:298: waitForThroughput.block(); On 2016/05/26 17:55:30, xunjieli wrote: > Is throughtput observation always going to be posted after the RTT observation? Yes.
lgtm mod comment below https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:298: waitForThroughput.block(); On 2016/05/26 18:02:59, tbansal1 wrote: > On 2016/05/26 17:55:30, xunjieli wrote: > > Is throughtput observation always going to be posted after the RTT > observation? > > Yes. Please add a comment above in "public void onThroughputObservation" of TestNetworkQualityListener to make a note of this. It is not obvious why you want to wait for throughput instead of both rtt and throughput.
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:150001) has been deleted
Addressed the comments.
https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/1999303002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:298: waitForThroughput.block(); On 2016/05/26 18:06:49, xunjieli wrote: > On 2016/05/26 18:02:59, tbansal1 wrote: > > On 2016/05/26 17:55:30, xunjieli wrote: > > > Is throughtput observation always going to be posted after the RTT > > observation? > > > > Yes. > > Please add a comment above in "public void onThroughputObservation" of > TestNetworkQualityListener to make a note of this. It is not obvious why you > want to wait for throughput instead of both rtt and throughput. I expanded on the comment here, instead of in onThroughputObservation. Let me know if this is still vague.
lgtm
Patchset #6 (id:190001) has been deleted
The previous version was still flaky because the observation counts were written and read by different threads. I added locks for synchronization.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/1999303002/#ps210001 (title: "Add locks so that counts can be read concurrently")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999303002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999303002/210001
Message was sent while issue was closed.
Description was changed from ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before that CL, the observations were available synchronously). BUG=614227 ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before that CL, the observations were available synchronously). BUG=614227 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:210001)
Message was sent while issue was closed.
Description was changed from ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before that CL, the observations were available synchronously). BUG=614227 ========== to ========== Fix flaky NQE cronet test Example of a flaky run from testbot: https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... and https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Bui... A recent NQE CL changed the logic so that throughput observations are now posted to NQE (before that CL, the observations were available synchronously). BUG=614227 Committed: https://crrev.com/86d66363a9aad84556298c81c4a5af935d67d03b Cr-Commit-Position: refs/heads/master@{#396488} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/86d66363a9aad84556298c81c4a5af935d67d03b Cr-Commit-Position: refs/heads/master@{#396488} |
