fix(spanner): prevent fastpath tablet routing flaps#13803
Conversation
Ignore group updates with older generations so stale tablet addresses cannot overwrite newer skip state. At equal generation, retain skip-and-empty tablets unless the incoming tablet incarnation is lexicographically newer, which permits legitimate recovery while rejecting frontend-local availability flaps. Gate endpoint recreation on active finder membership and hold the reconciliation lock through insertion so concurrent removal cannot resurrect an inactive address.
There was a problem hiding this comment.
Code Review
This pull request addresses race conditions and out-of-order updates in the Spanner client's endpoint lifecycle and key range cache. Specifically, it ensures that endpoint recreation requests do not race with active address removal by checking membership under a lock, and it prevents stale cache updates from overwriting newer tablet membership information by comparing generation and incarnation metadata. The review feedback recommends moving the clock.instant() call outside of the synchronized block in EndpointLifecycleManager to follow the 'open call' design principle, preventing potential deadlocks or performance bottlenecks.
| boolean stillActive = false; | ||
| boolean recreated = false; | ||
| // Check membership and insert under the reconciliation lock so active-set removal cannot | ||
| // complete between them and leave an inactive endpoint behind. | ||
| synchronized (activeAddressLock) { | ||
| for (Set<String> addresses : activeAddressesPerFinder.values()) { | ||
| if (addresses.contains(address)) { | ||
| stillActive = true; | ||
| break; | ||
| } | ||
| } | ||
| if (stillActive) { | ||
| EndpointState state = new EndpointState(address, clock.instant()); | ||
| recreated = endpoints.putIfAbsent(address, state) == null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Calling clock.instant() inside the synchronized (activeAddressLock) block violates the open call design principle (calling an overridable/external method while holding a lock). Since Clock is an overridable dependency, its implementation could block or acquire other locks, potentially leading to deadlocks or scalability bottlenecks.
Moving clock.instant() outside of the synchronized block avoids holding the lock during this call. This also makes the concurrent test more robust and deterministic, as the removal thread can finish cleanly without relying on thread blocking states.
Instant now = clock.instant();
boolean stillActive = false;
boolean recreated = false;
// Check membership and insert under the reconciliation lock so active-set removal cannot
// complete between them and leave an inactive endpoint behind.
synchronized (activeAddressLock) {
for (Set<String> addresses : activeAddressesPerFinder.values()) {
if (addresses.contains(address)) {
stillActive = true;
break;
}
}
if (stillActive) {
EndpointState state = new EndpointState(address, now);
recreated = endpoints.putIfAbsent(address, state) == null;
}
}| } | ||
|
|
||
| List<TabletSnapshot> tablets = new ArrayList<>(groupIn.getTabletsCount()); | ||
| for (int t = 0; t < groupIn.getTabletsCount(); t++) { |
There was a problem hiding this comment.
In this logic I think we will are allowing a skip=true to come after a skip=False and after that we will again allow skip=False becuase it will have incarnation number and cache wont
Ignore group updates with older generations so stale tablet addresses cannot overwrite newer skip state. At equal generation, retain skip-and-empty tablets unless the incoming tablet incarnation is lexicographically newer, which permits legitimate recovery while rejecting frontend-local availability flaps.
Gate endpoint recreation on active finder membership and hold the reconciliation lock through insertion so concurrent removal cannot resurrect an inactive address.