Sticky query routing via query options #12276

saurabhd336 · 2024-01-17T10:41:55Z

Allow deterministic and sticky query routing selection based on a query option. This supports cases where users may want to ensure consistent view of the data segments across multiple queries (in case of realtime tables, essentially route to the same server for a consuming segments, avoiding data inconsistency issues). This is still a best effort implementation, and fallback to other servers is possible in case of a server being down.

Release notes

Adds support for deterministic and sticky routing for a query / table / broker. This setting would lead to same server / set of servers (for MultiStageReplicaGroupSelector) being used for all queries of a given table.

Query option (takes precedence over fixed routing setting at table / broker config level)
SET "useFixedReplica"=true;

Table config (takes precedence over fixed routing setting at broker config level)

        "routing": {
                ...
                "useFixedReplica": true
        }
}

Broker conf

pinot.broker.use.fixed.replica=true

codecov-commenter · 2024-01-17T11:27:45Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (19e74e8) 61.58% compared to head (a122dfd) 61.58%.
Report is 9 commits behind head on master.

Files	Patch %	Lines
...routing/instanceselector/BaseInstanceSelector.java	62.50%	1 Missing and 2 partials ⚠️
...ing/instanceselector/BalancedInstanceSelector.java	50.00%	1 Missing and 1 partial ⚠️
...stanceselector/MultiStageReplicaGroupSelector.java	50.00%	1 Missing and 1 partial ⚠️
...instanceselector/ReplicaGroupInstanceSelector.java	50.00%	1 Missing and 1 partial ⚠️
...e/pinot/common/utils/config/QueryOptionsUtils.java	0.00%	2 Missing ⚠️
...ting/instanceselector/InstanceSelectorFactory.java	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #12276      +/-   ##
============================================
- Coverage     61.58%   61.58%   -0.01%     
  Complexity     1152     1152              
============================================
  Files          2417     2415       -2     
  Lines        131367   131502     +135     
  Branches      20262    20310      +48     
============================================
+ Hits          80907    80982      +75     
- Misses        44559    44587      +28     
- Partials       5901     5933      +32

Flag	Coverage Δ
custom-integration1	`<0.01% <0.00%> (ø)`
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`61.55% <62.50%> (+0.01%)`	⬆️
java-21	`34.64% <9.37%> (-26.83%)`	⬇️
skip-bytebuffers-false	`61.57% <62.50%> (-0.01%)`	⬇️
skip-bytebuffers-true	`34.62% <9.37%> (-26.80%)`	⬇️
temurin	`61.58% <62.50%> (-0.01%)`	⬇️
unittests	`61.57% <62.50%> (-0.01%)`	⬇️
unittests1	`46.60% <60.00%> (-0.02%)`	⬇️
unittests2	`27.79% <53.12%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

walterddr · 2024-01-17T19:28:12Z

...src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java

+        int selectedIdx;
+        if (_useStickyRouting || QueryOptionsUtils.isUseStickyRouting(queryOptions)) {
+          // candidates array is always sorted
+          selectedIdx = Math.abs(_tableNameWithType.hashCode() % candidates.size());


this sticks the request to a single broker instead of distributed a sticky routing across broker but maintain the same one for a single client. is this what we wanted?

I think you meant single server?
And yes, the requirement is to ensure same set of servers are picked for queries that are run with this query option set to true.
What we're trying to achieve is essentially ensure a consistent view of the data across queries which may be a requirement for a certain usecases.
Since the current set of instance selectors essentially round robin / randomly pick any of the avaliable servers, sometimes it's possible to see query result inconsistencies across queries due to for example the consuming segment on one of the replicas lagging behind another replica.

This option ensures we always route to same set of servers therefore for the particular subset of queries with this option set, always see a consistent view of the data (unless ofcourse the assignment changes / server restarts and is catching up / segments are deleted etc, which has been called out explicitly in the PR)

yes I meant server and the concern is with traffic skew b/c all sticky routing query are going to hit the same replica of the table (request is based on tableName hash)

if we dont have a clientID for sticky routing i dont think there's any other way to achieve this.

Jackie-Jiang

Let's also add a config in TableConfig.RoutingConfig, and allow 3 levels of config: instance -> table -> query

Jackie-Jiang · 2024-01-19T05:29:13Z

...src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java

-      BrokerMetrics brokerMetrics, @Nullable AdaptiveServerSelector adaptiveServerSelector, Clock clock) {
-    super(tableNameWithType, propertyStore, brokerMetrics, adaptiveServerSelector, clock);
+      BrokerMetrics brokerMetrics, @Nullable AdaptiveServerSelector adaptiveServerSelector, Clock clock,
+      boolean useStickyRouting) {


IMO sticky routing is not very clear, useSingleReplica or useFixedReplica might be more explicit. This is personal preference, so open to other names

Changed to consistent routing

There are still some places with useStickyRouting

Changed to useFixedReplica

Jackie-Jiang · 2024-01-19T05:30:25Z

...src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java

@@ -89,7 +91,13 @@ Pair<Map<String, String>, Map<String, String>> select(List<String> segments, int
        if (candidates == null) {
          continue;
        }
-        int selectedIdx = requestId++ % candidates.size();
+        int selectedIdx;
+        if (_useStickyRouting || QueryOptionsUtils.isUseStickyRouting(queryOptions)) {


We should allow query option to override this. Basically we can have broker config -> table config -> query options. Each level can override previous level if explicitly configured.

Added the support for overridng brokerConf with query options.

The problem I see with table level config is, we either have to fetch the table config on each query, or we need to somehow recreate the routing config each time table config changes or we need to call out that updating the table config setting for this feature needs a broker restart. Thoughts?

We use the table config when creating the routing. When table config is changed, it will trigger a routing rebuild to pick up the changes. We rely on the same logic for other routing configs.

Makes sense. I did not notice the RefreshTableConfigMessageHandler. Changed

Jackie-Jiang · 2024-01-19T05:35:00Z

...src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java

+        int selectedIdx;
+        if (_useStickyRouting || QueryOptionsUtils.isUseStickyRouting(queryOptions)) {
+          // candidates array is always sorted
+          selectedIdx = Math.abs(_tableNameWithType.hashCode() % candidates.size());


Consider using raw table name so that we can pick the same replica for hybrid use cases for future proof. Also we can cache the hash (positive value) to avoid calculating hash per query

Jackie-Jiang · 2024-01-19T05:35:59Z

.../src/main/java/org/apache/pinot/broker/routing/instanceselector/InstanceSelectorFactory.java

  public static InstanceSelector getInstanceSelector(TableConfig tableConfig,
-      ZkHelixPropertyStore<ZNRecord> propertyStore, BrokerMetrics brokerMetrics) {
-    return getInstanceSelector(tableConfig, propertyStore, brokerMetrics, null, Clock.systemUTC());
+      ZkHelixPropertyStore<ZNRecord> propertyStore, BrokerMetrics brokerMetrics, PinotConfiguration config) {


(minor) Suggest renaming to brokerConf to make it clear that it is top level broker config

Jackie-Jiang · 2024-01-19T05:38:12Z

...in/java/org/apache/pinot/broker/routing/instanceselector/MultiStageReplicaGroupSelector.java

+      // selection of replica group across queries i.e. same instance replica group id is picked each time.
+      // Since the instances within a selected replica group are iterated in order, the assignment within a selected
+      // replica group is guaranteed to be deterministic.
+      replicaGroupSelected = 0;


We should still start with the table hash to avoid always selecting the servers from the same replica. If we always pick replica 0, it will likely cause hotspot servers because servers with smaller id will always be picked

So the idea with the MultiStageReplicaGroupSelector is to ensure colocated tables get routed to same set of servers IIUC. If we use table hash here, that goal will not be achieved (the non sticky routing path uses requestId % and not tableName hash).

It's possible to

Use table hash here -> preferring even distribution across replica group for different tables over colocation.

Start from 0 -> preferring colocation at the cost of potential hotspots.

Not support stick routing when using the MultiStageReplicaGroupSelector

I went with 2, but open to suggestions here. cc: @walterddr

Good point. IIRC we are not using this selector, so both 2 and 3 (1 breaks the contract which is not okay) are fine. If we go with 2, let's add a note describing the potential hotspot problem, or we go with 3 and add a todo

Added a note comment

Jackie-Jiang · 2024-01-19T05:38:54Z

...src/main/java/org/apache/pinot/broker/routing/instanceselector/SegmentInstanceCandidate.java

@@ -41,4 +41,9 @@ public String getInstance() {
  public boolean isOnline() {
    return _online;
  }
+
+  @Override
+  public int compareTo(SegmentInstanceCandidate o) {


Is this a bugfix?

Bad commit. reverted.

Jackie-Jiang · 2024-01-19T05:39:44Z

pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java

@@ -145,6 +145,11 @@ public static boolean isSkipScanFilterReorder(Map<String, String> queryOptions)
    return "false".equalsIgnoreCase(queryOptions.get(QueryOptionKey.USE_SCAN_REORDER_OPTIMIZATION));
  }

+  @Nullable
+  public static boolean isUseStickyRouting(Map<String, String> queryOptions) {


We should return Boolean here, and differentiate not set and explicit false

I updated to use queryOption -> broker config fallback

Jackie-Jiang · 2024-01-19T22:19:32Z

Please take a look at the failed test

Jackie-Jiang

LGTM!

Jackie-Jiang · 2024-01-20T08:38:40Z

...ker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java

@@ -87,6 +89,8 @@ abstract class BaseInstanceSelector implements InstanceSelector {
  final BrokerMetrics _brokerMetrics;
  final AdaptiveServerSelector _adaptiveServerSelector;
  final Clock _clock;
+  final Boolean _useFixedReplica;


Suggested change

final Boolean _useFixedReplica;

final boolean _useFixedReplica;

Jackie-Jiang · 2024-01-20T08:40:06Z

...ker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java

+    _useFixedReplica = useFixedReplica;
+    // Using raw table name to ensure queries spanning across REALTIME and OFFLINE tables are routed to the same
+    // instance
+    _tableNameHashForFixedReplicaRouting = TableNameBuilder.extractRawTableName(tableNameWithType).hashCode();


(minor) We can keep a positive hash here, which avoids the per query abs

Jackie-Jiang · 2024-01-20T08:46:36Z

...ker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BaseInstanceSelector.java

@@ -429,6 +443,12 @@ public SelectionResult select(BrokerRequest brokerRequest, List<String> segments
    }
  }

+  protected boolean isUseFixedReplica(Map<String, String> queryOptions) {
+    return Boolean.parseBoolean(
+        queryOptions.getOrDefault(CommonConstants.Broker.Request.QueryOptionKey.USE_FIXED_REPLICA,


(minor) Suggest moving query option handling into QueryOptionUtils, and avoid per query string conversion:

Boolean useFixedReplicaFromQueryOption = QueryOptionUtils.getUseFixedReplica(queryOptions); return useFixedReplicaFromQueryOption != null ? useFixedReplicaFromQueryOption : _useFixedReplica;

Makes sense. Ack

* Support sticky routing via query options --------- Co-authored-by: Saurabh Dubey <saurabh.dubey@Saurabhs-MacBook-Pro.local>

Saurabh Dubey added 2 commits January 17, 2024 14:28

Support sticky routing via query options

7189556

Add comments for candidates array

ff9bd30

saurabhd336 requested review from Jackie-Jiang and snleee January 17, 2024 10:42

saurabhd336 requested a review from mayankshriv January 17, 2024 16:05

walterddr reviewed Jan 17, 2024

View reviewed changes

Add support in MultiStageReplicaGroupSelector

15c5a62

saurabhd336 force-pushed the stickyQueryRouting branch from b3a9f25 to 15c5a62 Compare January 18, 2024 05:05

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes Configuration Config changes (addition/deletion/change in behavior) labels Jan 19, 2024

Jackie-Jiang reviewed Jan 19, 2024

View reviewed changes

Review comments

6658386

Saurabh Dubey added 3 commits January 20, 2024 09:22

Review comments

58b52d5

Lint

c6a8549

Fix tests

d478685

mayankshriv approved these changes Jan 20, 2024

View reviewed changes

Fix test

6dbf619

Jackie-Jiang approved these changes Jan 20, 2024

View reviewed changes

Review comments

a122dfd

saurabhd336 merged commit 640ebe5 into apache:master Jan 20, 2024
19 checks passed

saurabhd336 added a commit to saurabhd336/pinot that referenced this pull request Feb 9, 2024

Sticky query routing via query options (apache#12276)

85de7b3

* Support sticky routing via query options --------- Co-authored-by: Saurabh Dubey <saurabh.dubey@Saurabhs-MacBook-Pro.local>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sticky query routing via query options #12276

Sticky query routing via query options #12276

saurabhd336 commented Jan 17, 2024 •

edited

codecov-commenter commented Jan 17, 2024 •

edited

walterddr Jan 17, 2024

saurabhd336 Jan 18, 2024

walterddr Jan 18, 2024

Jackie-Jiang left a comment

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 20, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 20, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 20, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang Jan 19, 2024

saurabhd336 Jan 19, 2024

Jackie-Jiang commented Jan 19, 2024

Jackie-Jiang left a comment

Jackie-Jiang Jan 20, 2024

saurabhd336 Jan 20, 2024

Jackie-Jiang Jan 20, 2024

saurabhd336 Jan 20, 2024

Jackie-Jiang Jan 20, 2024

saurabhd336 Jan 20, 2024

	final Boolean _useFixedReplica;
	final boolean _useFixedReplica;

Sticky query routing via query options #12276

Sticky query routing via query options #12276

Conversation

saurabhd336 commented Jan 17, 2024 • edited

Release notes

codecov-commenter commented Jan 17, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jackie-Jiang commented Jan 19, 2024

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhd336 commented Jan 17, 2024 •

edited

codecov-commenter commented Jan 17, 2024 •

edited