Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2143653005:
Dampening connection switch. (Closed)
DescriptionDampening connection switch.
If the currently selected connection becomes not receiving and if a backup connection
becomes strong first, we will not switch the connection until X milliseconds is passed
but the selected connection is still not receiving and the backup connection is still receiving. This will prevent the connection switching from happening too frequently.
BUG=
Committed: https://crrev.com/9ad0db51a679a5967b593e72431a10967b6182bb
Cr-Commit-Position: refs/heads/master@{#13480}
Patch Set 1 : Add receiving_dampening_interval as a IceConfig #Patch Set 2 : Add tests and fix an issue #
Total comments: 13
Patch Set 3 : Fix some comments #
Total comments: 19
Patch Set 4 : Address comments #
Total comments: 22
Patch Set 5 : Address comments #Patch Set 6 : Minor formatting #Patch Set 7 : . #
Total comments: 8
Patch Set 8 : . #
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds passes and the selected connection is still not receiving. BUG= ========== to ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds passes and the selected connection is still not receiving. This will prevent the connection switching from happening too frequently. BUG= ==========
Patchset #1 (id:1) has been deleted
honghaiz@webrtc.org changed reviewers: + pthatcher@webrtc.org, skvlad@google.com
You may want to take a look at the CL. I will add tests in parallel.
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Please hold this for a while. I am making some fixes to the CL.
Add tests and fix an issue. PTAL now.
https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:83: static const int RECEIVING_DAMPENING_INTERVAL = 500; // ms I think with at least three STUN pings on the selected connection, if the backup becomes receiving and the selected still is not receiving after 500ms, it is a pretty strong sign that the backup connection should be selected. Choosing 1000ms may adversely affecting normal connection switches that should be made.
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds passes and the selected connection is still not receiving. This will prevent the connection switching from happening too frequently. BUG= ========== to ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving. This will prevent the connection switching from happening too frequently. BUG= ==========
https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:701: int get_and_reset_selected_candidate_pair_switches() { Just call this reset_selected_candidate_pair_switches(). It's OK for a reset to return a value. https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2049: EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); Do we need to add to the test that after the expected dampening time, we *do* switch? https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1214: } I think this would be more clear if we had a method that both sets receiving_ to true and updates this other variable. For example: void Connection::SetReceivingNow(int64_t now) { set_receiving(true); if (first_received_since_channel_weak_ == 0) { first_received_since_channel_weak_ == now; } } Then you can just have this in each of the three methods: SetReceivingNow(now); By the way, I think we should also update the other place where we call set_receiving to be like this: void Connection::UpdateReceiving(int64_t now) { set_receiving(now <= last_received() + receiving_timeout_); } So that it just calls: UpdateReceiving(now) On the other hand, if we use my idea of continuously_receiving_since(), we could do that like so: void Connection::set_receiving(bool value int64_t now) { if (value != receiving_) { LOG_J(LS_VERBOSE, this) << "set_receiving to " << value; receiving_ = value; if (receiving_) { continuously_receiving_since_ = now; } else { continuously_receiving_since_ = INT64_MAX; // Or use rtc::optional } SignalStateChange(this); } } https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:595: // transport channel is weak. is weak => was weakly connected https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:200: rtc::Optional<int> receiving_dampening_interval; I think a better name might be receiving_switching_delay. In other words, it's the amount that we delay switching based on the receiving signal. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:83: static const int RECEIVING_DAMPENING_INTERVAL = 500; // ms I think we should be conservative with the value for now and stick with 1000ms. If we feel confident that we can lower it later, we can. But I think it's better to stick with a bigger value and then test going down rather than risk being too low. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:209: } I don't like the idea of ShouldSwitchSelectedConnection being non-const and changing state. And I don't think we need to store the pending_selected_connection_. I think there are alternatives that are more simple. For example, if we know we need to wait X ms to see if we're past the dampening period, we could just schedule a MSG_SORT_AND_UPDATE_STATE that will take care of switching, if it should. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:211: cmp = CompareConnectionsWithoutStates(selected_connection_, new_connection); I also think it would be more simple to keep it as one CompareConnections and not break it up into two (WithDampening and WithoutStates). https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:993: was_strong_ = is_strong; I think we can calculate the "has it been receiving long enough" logic independently for each Connection and avoid having to reach into the connection with things like reset_num_pings_sent() and reset_first_received_since_channel_weak(). For example, we can simply have a "continuously_receiving_since()" that give the timestamp of the time we went from non receiving -> receiving. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1094: } Doesn't this change the order so that connected vs not takes precedence over receiving? https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1101: const Connection* new_conn, CompareConnectionX should know about "selected" and "new" connections. I think it would make more sense to pass call them "a" and "b" and then have a "b_receiving_delay" passed in. Or better yet, combine "now" with "b_receiving_delay" into a "b_receiving_threshold". https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1117: *config_.receiving_dampening_interval) { I think it would make more sense to have the Connection have a method with something like continuously_receiving_since() that returns a timestamp of the earliest time of the currently continuous time span that it's been receiving. Then this could be: if (!a->receiving() && b->receiving() && b->continuously_receiving_since() < b_receiving_threshold) { return b_is_better; } return 0; https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1228: return CompareConnections(a, b) > 0; I think it would be less risky to leave the CompareConnections methods as-is (with the exception off adding the b_receiving_threhsold). https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1504: } This code, or the equivalent, really should be inside of UpdateConnectionStates.
PTAL. Thanks! https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:701: int get_and_reset_selected_candidate_pair_switches() { On 2016/07/13 21:17:22, pthatcher1 wrote: > Just call this reset_selected_candidate_pair_switches(). It's OK for a reset to > return a value. Done. https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2049: EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); On 2016/07/13 21:17:22, pthatcher1 wrote: > Do we need to add to the test that after the expected dampening time, we *do* > switch? There is a test above, TestFailoverControllingSide, where if the link does not recover, it will switch. Do you want to test that the Connection will switch to the backup one and then to the originally selected one? I think that at least the first part (switch to the backup one) has been tested in the test above. Basically it tests that it will switch within a timeline. https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:1214: } On 2016/07/13 21:17:22, pthatcher1 wrote: > I think this would be more clear if we had a method that both sets receiving_ to > true and updates this other variable. For example: > > void Connection::SetReceivingNow(int64_t now) { > set_receiving(true); > if (first_received_since_channel_weak_ == 0) { > first_received_since_channel_weak_ == now; > } > } > > > Then you can just have this in each of the three methods: > SetReceivingNow(now); > > > By the way, I think we should also update the other place where we call > set_receiving to be like this: > > void Connection::UpdateReceiving(int64_t now) { > set_receiving(now <= last_received() + receiving_timeout_); > } > > So that it just calls: > UpdateReceiving(now) > > > On the other hand, if we use my idea of continuously_receiving_since(), we could > do that like so: > > void Connection::set_receiving(bool value int64_t now) { > if (value != receiving_) { > LOG_J(LS_VERBOSE, this) << "set_receiving to " << value; > receiving_ = value; > if (receiving_) { > continuously_receiving_since_ = now; > } else { > continuously_receiving_since_ = INT64_MAX; // Or use rtc::optional > } > SignalStateChange(this); > } > } Done. I think it does not matter what value is set when is not receiving. Or actually better to be 0 which is helpful when resetting the value. https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:595: // transport channel is weak. On 2016/07/13 21:17:22, pthatcher1 wrote: > is weak => was weakly connected Method removed and replaced. https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/transport.h File webrtc/p2p/base/transport.h (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/transpor... webrtc/p2p/base/transport.h:200: rtc::Optional<int> receiving_dampening_interval; On 2016/07/13 21:17:22, pthatcher1 wrote: > I think a better name might be receiving_switching_delay. In other words, it's > the amount that we delay switching based on the receiving signal. Done. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:83: static const int RECEIVING_DAMPENING_INTERVAL = 500; // ms On 2016/07/13 21:17:23, pthatcher1 wrote: > I think we should be conservative with the value for now and stick with 1000ms. > If we feel confident that we can lower it later, we can. But I think it's > better to stick with a bigger value and then test going down rather than risk > being too low. There is actually some risk of choosing the same dampening value and the receiving timeout value. Because by the time we re-check it again the backup connection may become not receiving if only one stun-ping response message was received on the backup connection. This may cause some switching not happening while it should happen. I set it to 900ms for now. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:209: } On 2016/07/13 21:17:23, pthatcher1 wrote: > I don't like the idea of ShouldSwitchSelectedConnection being non-const and > changing state. > > And I don't think we need to store the pending_selected_connection_. I think > there are alternatives that are more simple. > > For example, if we know we need to wait X ms to see if we're past the dampening > period, we could just schedule a MSG_SORT_AND_UPDATE_STATE that will take care > of switching, if it should. Done for other parts except that ShouldSwitchSelectedConnection needs to schedule a event, which access this, which is not a const argument. So there is no easy way to keep the method const. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:211: cmp = CompareConnectionsWithoutStates(selected_connection_, new_connection); On 2016/07/13 21:17:23, pthatcher1 wrote: > I also think it would be more simple to keep it as one CompareConnections and > not break it up into two (WithDampening and WithoutStates). Done. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:993: was_strong_ = is_strong; On 2016/07/13 21:17:23, pthatcher1 wrote: > I think we can calculate the "has it been receiving long enough" logic > independently for each Connection and avoid having to reach into the connection > with things like reset_num_pings_sent() and > reset_first_received_since_channel_weak(). > > For example, we can simply have a "continuously_receiving_since()" that give the > timestamp of the time we went from non receiving -> receiving. We can do that. But a potential problem is that if the backup connection was receiving (for example due to the periodic ping message on the backup connection), it will switch right away. So I just reset the continuously_receiving_since to |now| if the selected connection becomes weak. I realize reset_num_pings_sent is not needed any more because if the switching is dampened, it will be in weak state and so it will be pinged in fast rate. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1094: } On 2016/07/13 21:17:22, pthatcher1 wrote: > Doesn't this change the order so that connected vs not takes precedence over > receiving? I did not think that should have a major impact although we don't have to change that in this CL. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1101: const Connection* new_conn, On 2016/07/13 21:17:23, pthatcher1 wrote: > CompareConnectionX should know about "selected" and "new" connections. I think > it would make more sense to pass call them "a" and "b" and then have a > "b_receiving_delay" passed in. Or better yet, combine "now" with > "b_receiving_delay" into a "b_receiving_threshold". Done https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1117: *config_.receiving_dampening_interval) { On 2016/07/13 21:17:22, pthatcher1 wrote: > I think it would make more sense to have the Connection have a method with > something like continuously_receiving_since() that returns a timestamp of the > earliest time of the currently continuous time span that it's been receiving. > Then this could be: > > if (!a->receiving() && b->receiving() && > b->continuously_receiving_since() < b_receiving_threshold) { > return b_is_better; > } > return 0; > Done, somewhat different though. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1228: return CompareConnections(a, b) > 0; On 2016/07/13 21:17:22, pthatcher1 wrote: > I think it would be less risky to leave the CompareConnections methods as-is > (with the exception off adding the b_receiving_threhsold). Done. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1504: } On 2016/07/13 21:17:23, pthatcher1 wrote: > This code, or the equivalent, really should be inside of UpdateConnectionStates. This serves the purpose of periodical checking the dampened connection. Now that is not needed, just remove this.
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2049: EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); On 2016/07/14 04:54:20, honghaiz3 wrote: > On 2016/07/13 21:17:22, pthatcher1 wrote: > > Do we need to add to the test that after the expected dampening time, we *do* > > switch? > There is a test above, TestFailoverControllingSide, > where if the link does not recover, it will switch. > Do you want to test that the Connection will switch to the backup one and then > to the originally selected one? > I think that at least the first part (switch to the backup one) has been tested > in the test above. Basically it tests that it will switch within a timeline. As long as that case is covered. Perhaps make a note of it here. https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/120001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:209: } On 2016/07/14 04:54:20, honghaiz3 wrote: > On 2016/07/13 21:17:23, pthatcher1 wrote: > > I don't like the idea of ShouldSwitchSelectedConnection being non-const and > > changing state. > > > > And I don't think we need to store the pending_selected_connection_. I think > > there are alternatives that are more simple. > > > > For example, if we know we need to wait X ms to see if we're past the > dampening > > period, we could just schedule a MSG_SORT_AND_UPDATE_STATE that will take care > > of switching, if it should. > Done for other parts except that > ShouldSwitchSelectedConnection needs to schedule a event, which access this, > which is not a const argument. > So there is no easy way to keep the method const. Can't the *caller* of ShouldSwitchSelectedConnection schedule the re-comparison? Then it can remain const. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:83: static const int RECEIVING_SWITCHING_DELAY = 900; // ms I don't quite follow the distinction between 900ms and 1000ms. Perhaps you can explain it in person. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:201: (*config_.receiving_switching_delay) + Don't we have to make sure config_.receiving_switching_delay is set? https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:202: new_connection->continuously_receiving_since(); I was thinking of it the other way around: int64_t now = rtc::TimeMillis(); int64_t new_connection_receiving_threshold = now; if (config_.receiving_switching_delay) { // If the delay is set, the connection must have been receiving since longer ago. new_connection_receiving_threshold -= *config_.receiving_switching_delay; } https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:212: } If ShoudlSwitchSelectedConnection also takes/returns a "bool* switching_dampened", then the caller of ShouldSwitchSelectedConnection can choose to schedule the re-sort. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:988: c->reset_continuously_receiving_since(now); I don't think we need this. Won't the dampening work without it? https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1044: } Might as well just leave this part unchanged, since it was more consistent with the rest of the code and it makes the diff smaller. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1053: rtc::TimeMillis() >= b_receiving_threshold) { With my alternate definition of "threshold", this would be: if (b->receiving_continuously_since() < b_receiving_threshold) { return b_is_better; } else { *switching_dampened = true; } Which I think is more clear. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:230: // that, sets |switching_dampened| to be true. to be true => to true Also, "switching_dampened" might be better named "b_missed_recieving_threshold" https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:903: if (receiving_ == is_receiving) { is_receiving => receiving https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:909: SignalStateChange(this); I like how you refactored this. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:594: // state. I think it should be more like the time at which we transitioned from not receiving -> receiving and haven't transitioned to not receiving since. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:662: int64_t continuously_receiving_since_ = 0; Well, that makes it sounds like we've been continuously receiving forever. Perhaps using an rtc::optional would be more careful not to get confused about that. Or make it 7FFF FFFF.
PTAL. https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel_unittest.cc (right): https://codereview.webrtc.org/2143653005/diff/100001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel_unittest.cc:2049: EXPECT_EQ(0, get_and_reset_selected_candidate_pair_switches()); On 2016/07/14 18:01:43, pthatcher1 wrote: > On 2016/07/14 04:54:20, honghaiz3 wrote: > > On 2016/07/13 21:17:22, pthatcher1 wrote: > > > Do we need to add to the test that after the expected dampening time, we > *do* > > > switch? > > There is a test above, TestFailoverControllingSide, > > where if the link does not recover, it will switch. > > Do you want to test that the Connection will switch to the backup one and then > > to the originally selected one? > > I think that at least the first part (switch to the backup one) has been > tested > > in the test above. Basically it tests that it will switch within a timeline. > > As long as that case is covered. Perhaps make a note of it here. Done. Notes at the top of the test. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:201: (*config_.receiving_switching_delay) + On 2016/07/14 18:01:44, pthatcher1 wrote: > Don't we have to make sure config_.receiving_switching_delay is set? We set the default value at the beginning and only update it if the new value is set. So it should be always set. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:202: new_connection->continuously_receiving_since(); On 2016/07/14 18:01:44, pthatcher1 wrote: > I was thinking of it the other way around: > > int64_t now = rtc::TimeMillis(); > int64_t new_connection_receiving_threshold = now; > if (config_.receiving_switching_delay) { > // If the delay is set, the connection must have been receiving since longer > ago. > new_connection_receiving_threshold -= *config_.receiving_switching_delay; > } Done. If it is not set, it will just get a default int value 0 if you try to access it. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:212: } On 2016/07/14 18:01:44, pthatcher1 wrote: > If ShoudlSwitchSelectedConnection also takes/returns a "bool* > switching_dampened", then the caller of ShouldSwitchSelectedConnection can > choose to schedule the re-sort. Done. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1044: } On 2016/07/14 18:01:44, pthatcher1 wrote: > Might as well just leave this part unchanged, since it was more consistent with > the rest of the code and it makes the diff smaller. Done. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:1053: rtc::TimeMillis() >= b_receiving_threshold) { On 2016/07/14 18:01:44, pthatcher1 wrote: > With my alternate definition of "threshold", this would be: > > if (b->receiving_continuously_since() < b_receiving_threshold) { > return b_is_better; > } else { > *switching_dampened = true; > } > > Which I think is more clear. Done. Use an optional now. We still need to check if b_receiving_threshold is set. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:230: // that, sets |switching_dampened| to be true. On 2016/07/14 18:01:44, pthatcher1 wrote: > to be true => to true > > Also, "switching_dampened" might be better named "b_missed_recieving_threshold" Done. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.cc File webrtc/p2p/base/port.cc (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:903: if (receiving_ == is_receiving) { On 2016/07/14 18:01:44, pthatcher1 wrote: > is_receiving => receiving Done. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.cc#... webrtc/p2p/base/port.cc:909: SignalStateChange(this); On 2016/07/14 18:01:44, pthatcher1 wrote: > I like how you refactored this. Acknowledged. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.h File webrtc/p2p/base/port.h (right): https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:594: // state. On 2016/07/14 18:01:44, pthatcher1 wrote: > I think it should be more like the time at which we transitioned from not > receiving -> receiving and haven't transitioned to not receiving since. Done. https://codereview.webrtc.org/2143653005/diff/180001/webrtc/p2p/base/port.h#n... webrtc/p2p/base/port.h:662: int64_t continuously_receiving_since_ = 0; On 2016/07/14 18:01:44, pthatcher1 wrote: > Well, that makes it sounds like we've been continuously receiving forever. > Perhaps using an rtc::optional would be more careful not to get confused about > that. Or make it 7FFF FFFF. As I said, the value should not be used at all if the connection is not in receiving state. So it does not matter what value we put there and it does not worth using an optional here. What about -1?
Description was changed from ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving. This will prevent the connection switching from happening too frequently. BUG= ========== to ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving and the backup connection is still receiving. This will prevent the connection switching from happening too frequently. BUG= ==========
lgtm Just some minor style things. https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:201: rtc::TimeMillis() - (*config_.receiving_switching_delay)); rtc::TimeMillis() - config_.receiving_switching_delay.value_or(0); https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:230: this, MSG_SORT_AND_UPDATE_STATE); Might as well wrap that in if (config_.receiving_switching_delay) { } https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:665: "nomination on the controlled side")) { I like how you wrote MaybeSwitchSelectedConnection. But can you either print both log statements in the method or out of the method. Having one case in and one case out is awkward. I'd prefer both out. https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:320: // receiving-unchanged-threshold. More accurately, if we should switch but didn't because it didn't hit the threshold.
https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.cc (right): https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:201: rtc::TimeMillis() - (*config_.receiving_switching_delay)); On 2016/07/14 23:28:20, pthatcher1 wrote: > rtc::TimeMillis() - config_.receiving_switching_delay.value_or(0); Done. https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:230: this, MSG_SORT_AND_UPDATE_STATE); On 2016/07/14 23:28:20, pthatcher1 wrote: > Might as well wrap that in > > if (config_.receiving_switching_delay) { > } Done. https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.cc:665: "nomination on the controlled side")) { On 2016/07/14 23:28:20, pthatcher1 wrote: > I like how you wrote MaybeSwitchSelectedConnection. > > But can you either print both log statements in the method or out of the method. > Having one case in and one case out is awkward. > > I'd prefer both out. If we put both out of the method, the message "Switchinng selected ...." will be printed after "the old connection ... the new connection ..." That will looks awkward. If we want both inside, the issue is that we don't log a message if it is not switched in all other calls. Actually I intend to remove this only case where it log a message when it does not switch. It does not seem to be very useful for debugging. We can discuss this later. For now, I just re-arranged it a little to hopefully make it look better. https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... File webrtc/p2p/base/p2ptransportchannel.h (right): https://codereview.webrtc.org/2143653005/diff/240001/webrtc/p2p/base/p2ptrans... webrtc/p2p/base/p2ptransportchannel.h:320: // receiving-unchanged-threshold. On 2016/07/14 23:28:20, pthatcher1 wrote: > More accurately, if we should switch but didn't because it didn't hit the > threshold. Done.
The CQ bit was checked by honghaiz@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org Link to the patchset: https://codereview.webrtc.org/2143653005/#ps150007 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving and the backup connection is still receiving. This will prevent the connection switching from happening too frequently. BUG= ========== to ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving and the backup connection is still receiving. This will prevent the connection switching from happening too frequently. BUG= ==========
Message was sent while issue was closed.
Committed patchset #8 (id:150007)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving and the backup connection is still receiving. This will prevent the connection switching from happening too frequently. BUG= ========== to ========== Dampening connection switch. If the currently selected connection becomes not receiving and if a backup connection becomes strong first, we will not switch the connection until X milliseconds is passed but the selected connection is still not receiving and the backup connection is still receiving. This will prevent the connection switching from happening too frequently. BUG= Committed: https://crrev.com/9ad0db51a679a5967b593e72431a10967b6182bb Cr-Commit-Position: refs/heads/master@{#13480} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9ad0db51a679a5967b593e72431a10967b6182bb Cr-Commit-Position: refs/heads/master@{#13480} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
