Create pit service layer changes #3921

bharath-techie · 2022-07-15T11:22:01Z

Signed-off-by: Bharathwaj G bharath78910@gmail.com

Description

Create point in time changes - merging the service layer changes as part of this PR and Rest changes will come in as part of separate PR.

Reference PR - Already reviewed create PIT API PR in feature branch -> #2745

Issues Resolved

#1147

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc 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.

github-actions · 2022-07-15T11:46:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/681/
CommitID: 361dc3736e4653fb9e51fd3442d3525318233af6

github-actions · 2022-07-15T11:58:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/682/
CommitID: 8a8f708

codecov-commenter · 2022-07-15T11:58:37Z

Codecov Report

Merging #3921 (5d8ec6b) into main (07775ff) will increase coverage by 0.13%.
The diff coverage is 64.44%.

@@             Coverage Diff              @@
##               main    #3921      +/-   ##
============================================
+ Coverage     70.57%   70.70%   +0.13%     
- Complexity    56679    56892     +213     
============================================
  Files          4563     4573      +10     
  Lines        272755   273256     +501     
  Branches      40040    40076      +36     
============================================
+ Hits         192505   193219     +714     
+ Misses        64014    63800     -214     
- Partials      16236    16237       +1

Impacted Files	Coverage Δ
...ark/time/NanoTimeVsCurrentTimeMillisBenchmark.java	`0.00% <ø> (ø)`
...a/org/opensearch/painless/antlr/PainlessLexer.java	`68.18% <ø> (ø)`
.../org/opensearch/painless/antlr/PainlessParser.java	`68.43% <ø> (ø)`
...earch/action/search/PitSearchContextIdForNode.java	`0.00% <0.00%> (ø)`
...ensearch/action/search/SearchContextIdForNode.java	`94.44% <ø> (ø)`
...er/src/main/java/org/opensearch/client/Client.java	`40.00% <ø> (ø)`
.../org/opensearch/client/support/AbstractClient.java	`35.21% <0.00%> (+0.07%)`	⬆️
...rg/opensearch/common/settings/ClusterSettings.java	`91.89% <ø> (ø)`
...pensearch/common/settings/IndexScopedSettings.java	`100.00% <ø> (ø)`
...rg/opensearch/action/search/CreatePitResponse.java	`14.14% <14.14%> (ø)`
... and 517 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d7a152...5d8ec6b. Read the comment docs.

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

github-actions · 2022-07-15T12:30:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/684/
CommitID: fa9946b

github-actions · 2022-07-15T13:03:12Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/685/
CommitID: 9356bd49536f51f39542b5382de2c8cff6160536

github-actions · 2022-07-15T14:38:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/686/
CommitID: a0b09ad5513ef00010d1674ff56b2865a3a2a2de

Bukhtawar · 2022-07-15T16:46:43Z

/cc: @sachinpkale for review

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

github-actions · 2022-07-16T07:51:10Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/716/
CommitID: bfcb50a

Bukhtawar · 2022-07-17T09:01:47Z

/cc : @sachinpkale for help with review

sachinpkale · 2022-07-18T11:24:42Z

server/src/main/java/org/opensearch/action/search/SearchContextIdForNode.java

@@ -50,7 +50,7 @@ public final class SearchContextIdForNode implements Writeable {
    private final ShardSearchContextId searchContextId;
    private final String clusterAlias;

-    SearchContextIdForNode(@Nullable String clusterAlias, String node, ShardSearchContextId searchContextId) {
+    public SearchContextIdForNode(@Nullable String clusterAlias, String node, ShardSearchContextId searchContextId) {


Why is it made public? All the new classes are in the same package, right?

addressed it

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

github-actions · 2022-07-18T16:54:30Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/736/
CommitID: f96c9d7

… into createpitservice

github-actions · 2022-07-19T04:32:02Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/765/
CommitID: 7f9d1b8

github-actions · 2022-07-19T05:05:35Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/766/
CommitID: 27dedb36097073922a6cd74bb07cf5b95e6be8f6

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

github-actions · 2022-07-19T05:31:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/768/
CommitID: 5d8ec6b

Bukhtawar · 2022-07-19T05:45:59Z

server/src/main/java/org/opensearch/action/search/CreatePitController.java

+    public CreatePitController(
+        CreatePitRequest request,
+        SearchTransportService searchTransportService,
+        ClusterService clusterService,
+        TransportSearchAction transportSearchAction,
+        NamedWriteableRegistry namedWriteableRegistry,
+        Task task,
+        ActionListener<CreatePitResponse> listener
+    ) {


Lets create a singleton CreatePitController and remove the CreatePitRequest from the constructor, it should be a part of execute request part

Bukhtawar · 2022-07-19T05:48:49Z

server/src/main/java/org/opensearch/search/SearchService.java

+     * This setting will help validate the max keep alive that can be set during creation or extension for a PIT reader context
+     */
+    public static final Setting<TimeValue> MAX_PIT_KEEPALIVE_SETTING = Setting.positiveTimeSetting(
+        "pit.max_keep_alive",


lets use point_in_time everywhere to be consistent

Bukhtawar · 2022-07-19T06:11:06Z

server/src/main/java/org/opensearch/search/internal/ReaderContext.java

+    protected AtomicLong getLastAccessTime() {
+        return lastAccessTime;
+    }


Why not just return lastAccessTime.get() that way its immutable?

We use this method to update the last access time

getLastAccessTime().updateAndGet(curr -> Math.max(curr, nowInMillis()));

Seems like a violation of the GET API

okay agreed, made the change to just update in base class.

Bukhtawar · 2022-07-19T06:14:21Z

server/src/main/java/org/opensearch/action/search/SearchUtils.java

+    /**
+     * Get connection lookup listener for list of clusters passed
+     */
+    public static StepListener<BiFunction<String, String, DiscoveryNode>> getConnectionLookupListener(


This doesn't look generic enough, espl that it is in a utility class. Can we have it return a plain ActionListener instead

github-actions · 2022-07-19T07:02:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/776/
CommitID: 2b84ef204b534c9dce51dfa46cf63d15dd2b0a46

github-actions · 2022-07-19T07:06:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/777/
CommitID: bf4c4eec0ab9df4f3597f0ceaf4f05d7c7711efa

github-actions · 2022-07-19T07:12:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/778/
CommitID: 6963b68278c9f760bdd7a943be2dde4059a24ae5

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

Bukhtawar · 2022-07-19T08:32:57Z

server/src/main/java/org/opensearch/action/search/SearchUtils.java

+        final StepListener<BiFunction<String, String, DiscoveryNode>> lookupListener = new StepListener<>();
+
+        if (clusters.isEmpty()) {
+            lookupListener.onResponse((cluster, nodeId) -> state.getNodes().get(nodeId));


For my understanding what is this call supposed to do

so this is a a predicate which will give the node info given the cluster alias and node id.

if clusters are empty, we will just give the node info from the current cluster state

otherwise we will use remote cluster service to collect nodes from all clusters mentioned and return the node based on cluster alias and node id

But aren't we calling the onResponse instead of overriding the onResponse for the listener like

new ActionListener() { public void onResponse() { blah } }

since this is step listener, we will return the listener and the callers will do whenComplete() to perform action after the node lookup. This is technically a synchronous flow since caller will wait for whenComplete .

public method1 { Listener listener = getConnListener(); listener.whenComplete(do something); }

github-actions · 2022-07-19T08:34:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/784/
CommitID: 987cb57

Bukhtawar · 2022-07-19T08:37:56Z

server/src/main/java/org/opensearch/index/shard/SearchOperationListener.java

+                try {
+                    listener.onNewPitContext(readerContext);
+                } catch (Exception e) {
+                    logger.warn("onNewPitContext listener failed", e);


Note single listener failure will abort all other listeners as well. Is this expected?

but since we only log and not throw exception , It will not abort other listeners right?

Bukhtawar · 2022-07-19T08:39:09Z

server/src/main/java/org/opensearch/search/DefaultSearchContext.java

+            int sliceLimit = indexService.getIndexSettings().getMaxSlicesPerPit();
+            int numSlices = sliceBuilder.getMax();
+            if (numSlices > sliceLimit) {
+                throw new IllegalArgumentException(


Should we throw OpenSearchRejectedException which should result in 429 instead of 5xx

i checked the enum, it does show 429 but that corresponds to "too many requests", should we still go ahead ?

github-actions · 2022-07-19T10:20:48Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/786/
CommitID: 8fcc25c

github-actions · 2022-07-19T10:36:52Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/787/
CommitID: ed20a79759da3705e5399ef24381d0799b65b9ef

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

github-actions · 2022-07-19T11:45:19Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/790/
CommitID: 8067e7b

bharath-techie force-pushed the createpitservice branch from 361dc37 to 8a8f708 Compare July 15, 2022 11:34

bharath-techie marked this pull request as ready for review July 15, 2022 12:03

bharath-techie requested review from a team and reta as code owners July 15, 2022 12:03

Create pit service layer changes

fa9946b

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the createpitservice branch from 8a8f708 to fa9946b Compare July 15, 2022 12:18

bharath-techie force-pushed the createpitservice branch from 9356bd4 to a0b09ad Compare July 15, 2022 14:18

changing to consistent action names

bfcb50a

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the createpitservice branch from a0b09ad to bfcb50a Compare July 16, 2022 07:27

sachinpkale reviewed Jul 18, 2022

View reviewed changes

sachinpkale approved these changes Jul 18, 2022

View reviewed changes

Addressing review comment

f96c9d7

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

Merge branch 'main' of https://github.com/opensearch-project/OpenSearch…

7f9d1b8

… into createpitservice

Addressing review comment

5d8ec6b

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the createpitservice branch from 27dedb3 to 5d8ec6b Compare July 19, 2022 05:08

Bukhtawar reviewed Jul 19, 2022

View reviewed changes

bharath-techie force-pushed the createpitservice branch 2 times, most recently from bf4c4ee to 6963b68 Compare July 19, 2022 06:49

Addressing review comments

987cb57

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the createpitservice branch from 6963b68 to 987cb57 Compare July 19, 2022 08:11

Bukhtawar reviewed Jul 19, 2022

View reviewed changes

Bukhtawar approved these changes Jul 19, 2022

View reviewed changes

bharath-techie force-pushed the createpitservice branch from 8fcc25c to ed20a79 Compare July 19, 2022 10:24

Addressing review comments

8067e7b

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the createpitservice branch from ed20a79 to 8067e7b Compare July 19, 2022 11:16

Bukhtawar merged commit 55eb86d into opensearch-project:main Jul 19, 2022

dhruv16dhr mentioned this pull request Jul 20, 2022

[META] Point In Time Search Issue List #3959

Open

47 tasks

bharath-techie mentioned this pull request Aug 1, 2022

Add changes for Create PIT and Delete PIT rest layer and rest high level client #4064

Merged

5 tasks

bharath-techie mentioned this pull request Sep 5, 2022

[Backport 2.x] [Point in time] Backport point in time changes #4406

Closed

6 tasks

bharath-techie mentioned this pull request Sep 28, 2022

[Backport 2.x] [Point in time] Backport point in time changes #4616

Merged

6 tasks

This was referenced Dec 27, 2022

[BUG] Fix new flaky test org.opensearch.search.DeletePitMultiNodeTests.testDeletePitWhileNodeDrop #4089

Closed

[BUG] Fix flaky org.opensearch.search.PitMultiNodeTests.testCreatePitWhileNodeDropWithAllowPartialCreationFalse test #4259

Open

Create pit service layer changes #3921

Create pit service layer changes #3921

Conversation

bharath-techie commented Jul 15, 2022 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jul 15, 2022 • edited Loading

Codecov Report

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 15, 2022

Gradle Check (Jenkins) Run Completed with:

Bukhtawar commented Jul 15, 2022

github-actions bot commented Jul 16, 2022

Gradle Check (Jenkins) Run Completed with:

Bukhtawar commented Jul 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

bharath-techie Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

bharath-techie commented Jul 15, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

bharath-techie Jul 19, 2022 •

edited

Loading