Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964603002:
JobController: do not create alternative job for those AlternativeServiceInfo (Closed)
|
Created:
3 years, 5 months ago by Zhongyi Shi Modified:
3 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionJobController: do not create alternative job for those AlternativeServiceInfo
whose advertised QUIC version does not contain any supported version by
the net stack.
If the origin has more than one AlternativeServiceInfo(s), the one with
no client support will be ingore. If no AlternativeServiceInfo has client
support, the JobController will skip creating QUIC alternative job.
BUG=738232
Review-Url: https://codereview.chromium.org/2964603002
Cr-Commit-Position: refs/heads/master@{#484373}
Committed: https://chromium.googlesource.com/chromium/src/+/ef3f4ce5a709ebccc5c88923c2fc45e969865af2
Patch Set 1 #
Total comments: 11
Patch Set 2 : Re #8 #Patch Set 3 : self review #Patch Set 4 : Always initialize local variable to fix windows compile issue #
Dependent Patchsets: Messages
Total messages: 36 (20 generated)
The CQ bit was checked by zhongyi@chromium.org to run a CQ dry run
Description was changed from ========== JobController: do not create alternative jobs for those AlternativeServiceInfo whose advertised QUIC version does not contain any supported version by the net stack. If the origin has more than one AlternativeServiceInfo(s), the one with no client support will be ingore. If no AlternativeServiceInfo has client support, the JobController will skip creating QUIC alternative job. BUG=738232 ========== to ========== JobController: do not create alternative job for those AlternativeServiceInfo whose advertised QUIC version does not contain any supported version by the net stack. If the origin has more than one AlternativeServiceInfo(s), the one with no client support will be ingore. If no AlternativeServiceInfo has client support, the JobController will skip creating QUIC alternative job. BUG=738232 ==========
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zhongyi@chromium.org changed reviewers: + rch@chromium.org
Ryan: PTAL! I wonder whether the force QUIC on will be affected by this bugfix. Do we still rely on HttpServerProperties to get AlternativeServiceInfo when tryin QUIC in the force on case?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:1139: QUIC_VERSION_UNSUPPORTED) Since we don't do anything with the return value from this method, should it just return bool? https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.h:269: // and supported by the net stack. If more than one QuicVersions are nit: can you remove "by the net stack" from this comment (and elsewhere). ... " and is supported." https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller_unittest.cc:709: // Test we do not create alternative job when the advertised versions in nit: avoid first person. "Test that the controller does not create an ..." https://codereview.chromium.org/2964603002/diff/1/net/quic/chromium/quic_netw... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1200: } Can you do the same trick as in your previous CL? https://codereview.chromium.org/2964603002/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1206: unsupported_version = QUIC_VERSION_39; ditto
Patchset #3 (id:40001) has been deleted
Thanks Ryan, comments addressed, PTAL! https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:1139: QUIC_VERSION_UNSUPPORTED) On 2017/06/30 02:45:11, Ryan Hamilton wrote: > Since we don't do anything with the return value from this method, should it > just return bool? I could change to return a boolean in this one, the reason I return a QuicVersion here is that this method can be reused when JobController selects the best QuicVersion and plumbs to alt Job constructor. https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.h (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.h:269: // and supported by the net stack. If more than one QuicVersions are On 2017/06/30 02:45:11, Ryan Hamilton wrote: > nit: can you remove "by the net stack" from this comment (and elsewhere). > > ... " and is supported." Done. https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller_unittest.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller_unittest.cc:709: // Test we do not create alternative job when the advertised versions in On 2017/06/30 02:45:11, Ryan Hamilton wrote: > nit: avoid first person. "Test that the controller does not create an ..." Done. https://codereview.chromium.org/2964603002/diff/1/net/quic/chromium/quic_netw... File net/quic/chromium/quic_network_transaction_unittest.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1200: } On 2017/06/30 02:45:11, Ryan Hamilton wrote: > Can you do the same trick as in your previous CL? Done. https://codereview.chromium.org/2964603002/diff/1/net/quic/chromium/quic_netw... net/quic/chromium/quic_network_transaction_unittest.cc:1206: unsupported_version = QUIC_VERSION_39; On 2017/06/30 02:45:11, Ryan Hamilton wrote: > ditto Done.
The CQ bit was checked by rch@chromium.org
lgtm https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... File net/http/http_stream_factory_impl_job_controller.cc (right): https://codereview.chromium.org/2964603002/diff/1/net/http/http_stream_factor... net/http/http_stream_factory_impl_job_controller.cc:1139: QUIC_VERSION_UNSUPPORTED) On 2017/06/30 21:08:43, Zhongyi Shi wrote: > On 2017/06/30 02:45:11, Ryan Hamilton wrote: > > Since we don't do anything with the return value from this method, should it > > just return bool? > > I could change to return a boolean in this one, the reason I return a > QuicVersion here is that this method can be reused when JobController selects > the best QuicVersion and plumbs to alt Job constructor. Makes sense.
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
zhongyi@chromium.org changed reviewers: + mef@chromium.org
mef@: components/ please!
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by mef@chromium.org
lgtm components/ lgtm
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
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by zhongyi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org, rch@chromium.org Link to the patchset: https://codereview.chromium.org/2964603002/#ps80001 (title: "Always initialize local variable to fix windows compile issue")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by zhongyi@chromium.org
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
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by zhongyi@chromium.org
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": 1499293300777020,
"parent_rev": "e9be2af23b67c48d8e92201b8eee0e47ea725a6e", "commit_rev":
"ef3f4ce5a709ebccc5c88923c2fc45e969865af2"}
Message was sent while issue was closed.
Description was changed from ========== JobController: do not create alternative job for those AlternativeServiceInfo whose advertised QUIC version does not contain any supported version by the net stack. If the origin has more than one AlternativeServiceInfo(s), the one with no client support will be ingore. If no AlternativeServiceInfo has client support, the JobController will skip creating QUIC alternative job. BUG=738232 ========== to ========== JobController: do not create alternative job for those AlternativeServiceInfo whose advertised QUIC version does not contain any supported version by the net stack. If the origin has more than one AlternativeServiceInfo(s), the one with no client support will be ingore. If no AlternativeServiceInfo has client support, the JobController will skip creating QUIC alternative job. BUG=738232 Review-Url: https://codereview.chromium.org/2964603002 Cr-Commit-Position: refs/heads/master@{#484373} Committed: https://chromium.googlesource.com/chromium/src/+/ef3f4ce5a709ebccc5c88923c2fc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/ef3f4ce5a709ebccc5c88923c2fc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
