Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LWT and rack-aware routing bugfixes #1037

Merged
merged 5 commits into from
Jul 11, 2024

Conversation

wprzytula
Copy link
Collaborator

Problem solved

I've noticed two bugs in the routing code in DefaultPolicy:

  1. rack+latency-awareness bug.
    A copy-paste kind of bug made local rack non-replica nodes not present in the fallback iterator earlier than non-local-rack non-replica nodes. This was a fairly edge case bug, though, because for it to manifest, the following conditions would have to be satisfied:

    1. a non-replica node from the preferred rack must be penalised by LatencyAwareness,
    2. a non-replica node from the preferred DC but not preferred rack must be penalised by LatencyAwareness, too.
  2. LWT + no preferred DC + the primary replica down or disabled - bug.
    There is a special logic in default policy that handles the case when computing the first acceptable replica (i.e. such replica that it satisfies pick_predicate) is expensive (i.e. requires allocation). That logic was to make pick return None, because then fallback would allocate and compute all acceptable replicas. Unfortunately, the logic contained a bug that made pick continue execution instead of returning None, leading to a non-necessarily-replica being returned. This would break the LWT optimisation, because in case that the primary replica is filtered out by LB (e.g. it's down, or disabled by HostFilter), the second replica should be targeted instead, deterministically.

    The fix involves creating an enum to distinguish between three scenarios:

    1. No replica exists that could satisfy the pick predicate -> None;
    2. The primary replica satisfies the pick predicate -> Some(Computed(primary_replica));
    3. The primary replica does not satisfy the pick predicate, but it's
      possible that another replica does -> Some(ToBeComputedInFallback).

    Before the fix, the third scenario would merge with the first, leading to incorrect behaviour of not returning None from pick.

Bonus

Additionally, this PR comes with typo fixes, comment amendments, and increased test assertions verbosity to aid debugging.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have provided docstrings for the public items that I want to introduce.
  • [ ] I have adjusted the documentation in ./docs/source/.
  • [ ] I added appropriate Fixes: annotations to PR description.

They aid in debugging.
A copy-paste kind of bug made local rack non-replica nodes not present
in the fallback iterator earlier than non-local-rack non-replica nodes.
This was a fairly edge case bug, though, because for it to manifest,
the following condition would have to be satisfied:
1. a non-replica node from the preferred rack would have to be penalised
   by LatencyAwareness,
2. a non-replica node from the preferred DC but not preferred rack
   would have to be penalised by LatencyAwareness, too.
@wprzytula wprzytula added bug Something isn't working area/load-balancing labels Jul 10, 2024
@wprzytula wprzytula self-assigned this Jul 10, 2024
@wprzytula wprzytula requested review from Lorak-mmk and muzarski and removed request for Lorak-mmk July 10, 2024 05:50
Copy link

github-actions bot commented Jul 10, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 9407afb

@Lorak-mmk Lorak-mmk self-assigned this Jul 10, 2024
@muzarski muzarski self-assigned this Jul 10, 2024
There is a special logic in default policy that handles the case when
computing the first acceptable replica (i.e. such replica that it
satisfies `pick_predicate`) is expensive (i.e. requires allocation).
That logic was to make `pick` return None, because then `fallback`
would allocate and compute all acceptable replicas. Unfortunately,
the logic contained a bug that made `pick` continue execution instead
of returning None, leading to a non-necessarily-replica being returned.
This would break the LWT optimisation, because in case that the primary
replica is filtered out by LB (e.g. it's down, or disabled by
HostFilter), the second replica should be targeted instead,
deterministically.

The fix involves creating an enum to distinguish between three
scenarios:
1. No replica exists that could satisfy the pick predicate -> None;
2. The primary replica satisfies the pick predicate ->
   Some(Computed(primary_replica));
3. The primary replica does not satisfy the pick predicate, but it's
   possible that another replica does -> Some(ToBeComputedInFallback).

Before the fix, the third scenario would merge with the first,
leading to incorrect behaviour of not returning None from `pick`.
The test runs against ClusterData with node F being disabled by
HostFilter. In such arrangement, F should be never returned. As F is
the primary replica for executed statement and no DC is preferred,
the second replica (`A`) cannot be computed cheaply, so `pick` should
return None. Before the bug was fixed, `pick` would return an arbitrary
robinned node, e.g. `B` (not even a replica).
@wprzytula wprzytula force-pushed the lwt-routing-bugfix branch from d29464d to 9407afb Compare July 11, 2024 11:51
@wprzytula wprzytula requested a review from muzarski July 11, 2024 11:51
@wprzytula wprzytula merged commit c01ad2b into scylladb:main Jul 11, 2024
11 checks passed
wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
LWT and rack-aware routing bugfixes

(cherry picked from commit c01ad2b)
@wprzytula wprzytula mentioned this pull request Jul 11, 2024
wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
LWT and rack-aware routing bugfixes

(cherry picked from commit c01ad2b)
wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
LWT and rack-aware routing bugfixes

(cherry picked from commit c01ad2b)
wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this pull request Jul 11, 2024
LWT and rack-aware routing bugfixes

(cherry picked from commit c01ad2b)
@wprzytula wprzytula deleted the lwt-routing-bugfix branch August 20, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/load-balancing bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants