Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2464263003:
Limit exponential backoff for broken alternative services. (Closed)
DescriptionLimit exponential backoff for broken alternative services.
This is to limit overflow in shift.
BUG=660617
Committed: https://crrev.com/3c0589bd98e3022091ee6fd899fe218182424ccc
Cr-Commit-Position: refs/heads/master@{#430382}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Re: #7. #
Total comments: 4
Patch Set 3 : Clarify comment. #Messages
Total messages: 21 (12 generated)
The CQ bit was checked by bnc@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.
bnc@chromium.org changed reviewers: + zhongyi@chromium.org
Cherie: PTAL. Ryan: FYI. Thank you.
https://codereview.chromium.org/2464263003/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2464263003/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:479: ++count; What if the count >= kBrokenDelayMaxShift? I think you might want to then set the count = kBrokenDelayMaxShift + 1 (note the shift is actually count - 1) to limit the shift not beyond the range. That also suggests to rename the variable from count to shift.
The CQ bit was checked by bnc@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.
Thanks for fixing this! https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:478: int shift = recently_broken_alternative_services_[alternative_service] - 1; typo here? Since you're directly retrieving shift, I think what you need here is: int shift = recently_broken_alternative_services_[alternative_service] ; as in the old code, the prefix increment happens before the assignment. Sorry for not catching this in the first place.
rch@chromium.org changed reviewers: + rch@chromium.org
https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:26: // kBrokenAlternativeProtocolDelaySecs << kBrokenDelayMaxShift ~ 2 days. nit: I'm not sure what this comment means? Perhaps a static_assert?
PTAL, thank you. https://codereview.chromium.org/2464263003/diff/1/net/http/http_server_proper... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2464263003/diff/1/net/http/http_server_proper... net/http/http_server_properties_impl.cc:479: ++count; On 2016/11/02 20:17:58, Zhongyi Shi wrote: > What if the count >= kBrokenDelayMaxShift? I think you might want to then set > the count = kBrokenDelayMaxShift + 1 (note the shift is actually count - 1) to > limit the shift not beyond the range. That also suggests to rename the variable > from count to shift. Done. https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... File net/http/http_server_properties_impl.cc (right): https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:26: // kBrokenAlternativeProtocolDelaySecs << kBrokenDelayMaxShift ~ 2 days. On 2016/11/03 16:16:22, Ryan Hamilton wrote: > nit: I'm not sure what this comment means? Perhaps a static_assert? Done. https://codereview.chromium.org/2464263003/diff/20001/net/http/http_server_pr... net/http/http_server_properties_impl.cc:478: int shift = recently_broken_alternative_services_[alternative_service] - 1; On 2016/11/03 15:59:37, Zhongyi Shi wrote: > typo here? > > Since you're directly retrieving shift, I think what you need here is: > int shift = recently_broken_alternative_services_[alternative_service] ; > as in the old code, the prefix increment happens before the assignment. Sorry > for not catching this in the first place. Indeed prefix increment returns new value. And |count| has the new value in the old code. And |shift = count - 1|. There's many different ways of writing it. For example, I can write: int count = ++r[a]; int shift = count - 1; Or I can write int shift = r[a]++; Or I can write int shift = r[a]; ++r[a]; Or I can write ++r[a]; int shift = r[a] - 1; These are all equivalent, and I think this is the functionality that want. Do you have any preference? Or I can leave everything as is, and not introduce |shift|, but then it's not very clear how |count| interacts with |kBrokenDelayMaxShift| (one is count, the other is shift, and they are off by one).
lgtm
The CQ bit was checked by bnc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Limit exponential backoff for broken alternative services. This is to limit overflow in shift. BUG=660617 ========== to ========== Limit exponential backoff for broken alternative services. This is to limit overflow in shift. BUG=660617 Committed: https://crrev.com/3c0589bd98e3022091ee6fd899fe218182424ccc Cr-Commit-Position: refs/heads/master@{#430382} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3c0589bd98e3022091ee6fd899fe218182424ccc Cr-Commit-Position: refs/heads/master@{#430382} |
