Add _primary preference only for segment replication enabled indices #2040

dreamer-89 · 2023-08-30T02:02:47Z

Description

Adds _primary preference only for segment replication enabled indices.

Issues Resolved

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Suraj Singh <[email protected]>

codecov · 2023-08-30T02:24:53Z

Codecov Report

Merging #2040 (f9e6ee0) into main (56bc7d5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##               main    #2040   +/-   ##
=========================================
  Coverage     97.28%   97.28%           
  Complexity     4624     4624           
=========================================
  Files           409      409           
  Lines         11942    11943    +1     
  Branches        828      828           
=========================================
+ Hits          11618    11619    +1     
  Misses          317      317           
  Partials          7        7

Flag	Coverage Δ
sql-engine	`97.28% <100.00%> (+<0.01%)`	⬆️

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

Files Changed	Coverage Δ
...s/storage/OpenSearchDataSourceMetadataStorage.java	`98.75% <100.00%> (+<0.01%)`	⬆️

Signed-off-by: Suraj Singh <[email protected]>

penghuo · 2023-08-30T17:59:56Z

...rc/main/java/org/opensearch/sql/datasources/storage/OpenSearchDataSourceMetadataStorage.java

    SearchRequest searchRequest = new SearchRequest();
    searchRequest.indices(DATASOURCE_INDEX_NAME);
    SearchSourceBuilder searchSourceBuilder = new SearchSourceBuilder();
    searchSourceBuilder.query(query);
    searchSourceBuilder.size(DATASOURCE_QUERY_RESULT_SIZE);
    searchRequest.source(searchSourceBuilder);
+    if (state.isSegmentReplicationEnabled(DATASOURCE_INDEX_NAME)) {


why not add parameter in search Request? e.g. consistentRead = true. Current approach expose internal implementation detail to client. In future, in case we add any feature breaking consistent read, all the client require consistent read need to make code change accordingly.

This is a good point @penghuo ! I feel the existing parameter _primary serves the purpose for consistent reads and adding a new param can be confusing? I also agree isSegmentReplicationEnabled naming give insights to underlying replication strategy but this utility was initially develop for core internal usage and is re-used for plugins.

+1 on Peng's comment.
Also are we doing this with all the plugins?
Now, I am really not sure is it any worth of identifying the replication strategy and set the primary preference. What is the worst that could happen if we set the primary preference it for all domains? It overwhelms the primary shard only if we cross certain limit?
By any chance do we have numbers for that?

Adding a consistency param would be cleaner here for clients/plugins and doesn't expose implementation detail or use internal apis. However, this needs more vetting before being made api.

We need to define the behavior here - does this simply route to primary or does it guarantee a strong read every time it is invoked? Because if its the latter we need to perform a refresh internally to guarantee even the primary is serving the latest writes irrespective of what refresh policy may have been set on the write path. This concerns me because it changes refresh behavior where it may not be intended and have consequences. However, this is similar to the behavior of a GetRequest with realtime set as true so may be acceptable.

I think what we really are asking for here is the former, which is to "read the last refreshed on data" whatever that may be at the time, which before SR existed is default behavior.

We don't have any data on this @vamsi-amazon so I agree with leaving as is. The downside of primary preference is the request will fail if the primary is unavailable and not route to replicas that are healthy.

To alleviate some concern, I suggest we use _primary_first which will attempt primary first and fallback to replicas only if the node is weighed away.

@penghuo : Adding a new parameter may need more discussion and product feedback.
@vamsi-amazon : No, we don't have data around primary shard limits i.e. max request that it can support before causing instability.

Proceeding with @mch2 suggestion here.

Updated. Request for review @penghuo @vamsi-amazon @mch2

@penghuo @vamsi-amazon : Please have a look. Let know if you have any concerns.

CC @mch2

Signed-off-by: Suraj Singh <[email protected]>

dreamer-89 · 2023-09-01T19:17:34Z

@penghuo @vamsi-amazon : Can you please help with review and merge.

dreamer-89 · 2023-09-01T19:27:02Z

@vamsi-amazon @penghuo : Can you please add backport 2.x label to have this backport to 2.x/2.10 branch

…2040) * Add _primary preference only for segment replication enabled indices Signed-off-by: Suraj Singh <[email protected]> * Add test for segment replication tests Signed-off-by: Suraj Singh <[email protected]> * Remove isSegRepEnabled check and use _primary_first preference Signed-off-by: Suraj Singh <[email protected]> * Remove unused ClusterState reference Signed-off-by: Suraj Singh <[email protected]> * Self review Signed-off-by: Suraj Singh <[email protected]> --------- Signed-off-by: Suraj Singh <[email protected]> (cherry picked from commit 9c12628) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…2040) (#2045) * Add _primary preference only for segment replication enabled indices * Add test for segment replication tests * Remove isSegRepEnabled check and use _primary_first preference * Remove unused ClusterState reference * Self review --------- (cherry picked from commit 9c12628) Signed-off-by: Suraj Singh <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Add _primary preference only for segment replication enabled indices

a5c6b31

Signed-off-by: Suraj Singh <[email protected]>

dreamer-89 requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, vamsimanohar, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners August 30, 2023 02:02

Add test for segment replication tests

83df304

Signed-off-by: Suraj Singh <[email protected]>

penghuo reviewed Aug 30, 2023

View reviewed changes

Remove isSegRepEnabled check and use _primary_first preference

02825b3

Signed-off-by: Suraj Singh <[email protected]>

dreamer-89 force-pushed the Issue-1801 branch from 8390410 to 02825b3 Compare August 30, 2023 20:16

dreamer-89 added 2 commits August 30, 2023 13:18

Remove unused ClusterState reference

1980dda

Signed-off-by: Suraj Singh <[email protected]>

Self review

f9e6ee0

Signed-off-by: Suraj Singh <[email protected]>

penghuo approved these changes Sep 1, 2023

View reviewed changes

vamsimanohar approved these changes Sep 1, 2023

View reviewed changes

vamsimanohar merged commit 9c12628 into opensearch-project:main Sep 1, 2023

vamsimanohar added the backport 2.x label Sep 1, 2023

opensearch-trigger-bot bot mentioned this pull request Sep 1, 2023

[Backport 2.x] Add _primary preference only for segment replication enabled indices #2045

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add _primary preference only for segment replication enabled indices #2040

Add _primary preference only for segment replication enabled indices #2040

dreamer-89 commented Aug 30, 2023

codecov bot commented Aug 30, 2023 •

edited

Loading

penghuo Aug 30, 2023

dreamer-89 Aug 30, 2023 •

edited

Loading

vamsimanohar Aug 30, 2023 •

edited

Loading

mch2 Aug 30, 2023 •

edited

Loading

mch2 Aug 30, 2023

dreamer-89 Aug 30, 2023

dreamer-89 Aug 30, 2023

dreamer-89 Aug 31, 2023

dreamer-89 commented Sep 1, 2023

dreamer-89 commented Sep 1, 2023

Add _primary preference only for segment replication enabled indices #2040

Add _primary preference only for segment replication enabled indices #2040

Conversation

dreamer-89 commented Aug 30, 2023

Description

Issues Resolved

Check List

codecov bot commented Aug 30, 2023 • edited Loading

Codecov Report

penghuo Aug 30, 2023

Choose a reason for hiding this comment

dreamer-89 Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

vamsimanohar Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

mch2 Aug 30, 2023 • edited Loading

Choose a reason for hiding this comment

mch2 Aug 30, 2023

Choose a reason for hiding this comment

dreamer-89 Aug 30, 2023

Choose a reason for hiding this comment

dreamer-89 Aug 30, 2023

Choose a reason for hiding this comment

dreamer-89 Aug 31, 2023

Choose a reason for hiding this comment

dreamer-89 commented Sep 1, 2023

dreamer-89 commented Sep 1, 2023

codecov bot commented Aug 30, 2023 •

edited

Loading

dreamer-89 Aug 30, 2023 •

edited

Loading

vamsimanohar Aug 30, 2023 •

edited

Loading

mch2 Aug 30, 2023 •

edited

Loading