Thanks for finding and fixing this!

I think the second part of this change is exactly right, but I don't think the first part is. It's not clear to me that it actually solves any problem, and it looks like it's not actually necessary.

Please let me know if you have any questions. Thanks!

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @donnadionne)

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 368 at r2 (raw file):

    gpr_log(GPR_INFO,
            "[priority_lb %p] state update for priority %d, child %s, current "
            "priority %d",

This should be %u instead of %d, since the value is unsigned.

Looks like this same bug already exists on the previous line, and probably elsewhere in the file. Can you do a quick audit to fix these as long as you're in here? Thanks!

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 373 at r2 (raw file):

This change looks wrong to me. I don't think we should call TryNextPriorityLocked() if this state change is for a priority lower than the current priority. If the current priority is READY, then the state change for a lower-priority child is irrelevant. And if the current priority is not READY, then we will already have called TryNextPriorityLocked() when we got the state change from the current priority. In either case, calling TryNextPriorityLocked() for a lower-priority child doesn't make sense.

Is this change actually necessary to fix the bug you were seeing? If I understand the problem correctly, I think the change you made in TryNextPriorityLocked() should be enough to fix the problem by itself.

If the other change is not enough to fix the bug by itself, then I'd like to understand the exact sequence of events that's triggering the bug.

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 418 at r2 (raw file):

Resetting current_priority_ at the top of this function makes sense (and I think that's actually the real bug here). However, I think resetting current_child_from_before_update_ should be left where it was, because until we actually give up on connecting to any of the children in the new update, we want HandleChildConnectivityStateChangeLocked() to continue to be able to pass picker updates for that child up to our parent.

test/cpp/end2end/xds_end2end_test.cc, line 6344 at r2 (raw file):

  balancers_[0]->ads_service()->SetEdsResource(
      BuildEdsResource(args, DefaultEdsServiceName()));
  WaitForBackend(3, true);

No need for the second parameter here, since that's the default value.

fixed! you are right the first change was not needed. After the second change, current_prioirty_ gets updated properly in SelectPriorityLocked.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @donnadionne and @markdroth)

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 368 at r2 (raw file):

fixed all in the file

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 373 at r2 (raw file):

yes, just the one line chagne in TryNextPrioirtyLocked to unset the current_prioirity_ is enough

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 387 at r2 (raw file):

correct, it will not be entered after the unset

src/core/ext/filters/client_channel/lb_policy/priority/priority.cc, line 418 at r2 (raw file):

ok, just one line change, the other line restored

test/cpp/end2end/xds_end2end_test.cc, line 6344 at r2 (raw file):

Done.

Looks great! Thanks for fixing this!

Reviewed 2 of 2 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @donnadionne)