Implementing Delete PIT service layer changes by bharath-techie · Pull Request #3949 · opensearch-project/OpenSearch

bharath-techie · 2022-07-19T15:13:05Z

Description

Delete 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 delete PIT API PR in feature branch ->
#3401

Request:

`DELETE /_search/point_in_time

{
    "pit_id": [
        "o463QQEPbXktaW5kZXgtMDAwMDAxFkhGN09fMVlPUkVPLXh6MUExZ1hpaEEAFjBGbmVEZHdGU1EtaFhhUFc4ZkR5cWcAAAAAAAAAAAEWaXBPNVJtZEhTZDZXTWFFR05waXdWZwEWSEY3T18xWU9SRU8teHoxQTFnWGloQQAA",
        "o463QQEPbXktaW5kZXgtMDAwMDAxFkhGN09fMVlPUkVPLXh6MUExZ1hpaEEAFjBGbmVEZHdGU1EtaFhhUFc4ZkR5cWcAAAAAAAAAAAIWaXBPNVJtZEhTZDZXTWFFR05waXdWZwEWSEY3T18xWU9SRU8teHoxQTFnWGloQQAA"
    ]
}`

(or) Delete All :
`DELETE /_search/point_in_time/_all`

Response:

`{
    "pits": [
        {
            "succeeded": true,
            "pitId": "o463QQEPbXktaW5kZXgtMDAwMDAxFkhGN09fMVlPUkVPLXh6MUExZ1hpaEEAFjBGbmVEZHdGU1EtaFhhUFc4ZkR5cWcAAAAAAAAAAAEWaXBPNVJtZEhTZDZXTWFFR05waXdWZwEWSEY3T18xWU9SRU8teHoxQTFnWGloQQAA"
        },
        {
            "succeeded": false,
            "pitId": "o463QQEPbXktaW5kZXgtMDAwMDAxFkhGN09fMVlPUkVPLXh6MUExZ1hpaEEAFjBGbmVEZHdGU1EtaFhhUFc4ZkR5cWcAAAAAAAAAAAIWaXBPNVJtZEhTZDZXTWFFR05waXdWZwEWSEY3T18xWU9SRU8teHoxQTFnWGloQQAA"
        }
    ]
}
`

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.

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

…ch into deletepitservice

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

… into createpitservice

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

…ch into deletepitservice

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

…ch into deletepitservice

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

github-actions · 2022-07-19T15:37:50Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-07-19T15:41:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/795/
CommitID: 41d342d

Bukhtawar · 2022-07-19T15:53:34Z

/cc: @sachinpkale

sachinpkale · 2022-07-20T03:52:58Z

We use squash and merge while merging the PRs. In such cases, PR title becomes commit message in main branch. Use imperative mood to write PR title.

sachinpkale · 2022-07-20T03:57:29Z

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

+    /**
+     * @return Whether the attempt to delete PIT was successful.
+     */
+


nit: Extra line

sachinpkale · 2022-07-20T04:00:25Z

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

+
+    @Override
+    public RestStatus status() {
+        if (deletePitResults.isEmpty()) return NOT_FOUND;


What would be the status if PIT ID exists but we are not able to delete it?

Succeeded would be false for the PIT ID deletion failures as given in PR description. Status is still 200.

sachinpkale · 2022-07-20T04:03:28Z

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

 * @opensearch.internal
 */
-final class SearchContextIdForNode implements Writeable {
+public final class SearchContextIdForNode implements Writeable {


Repeat comment: Why is this made public?

It is used here - https://github.com/opensearch-project/OpenSearch/pull/3949/files#diff-2bca0e4d486c23e0913a6dc6e5481350180239b2a176cda1544ddb3c51b565c0R1440

sachinpkale · 2022-07-20T04:06:54Z

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

        ClusterService clusterService,
        TransportSearchAction transportSearchAction,
        NamedWriteableRegistry namedWriteableRegistry,
+        PitService pitService,


PitService is not used in this class, right?

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

github-actions · 2022-07-20T05:23:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/812/
CommitID: 25bb645

codecov-commenter · 2022-07-20T05:25:06Z

Codecov Report

Merging #3949 (ae618ff) into main (b24b02f) will decrease coverage by 0.01%.
The diff coverage is 76.45%.

@@             Coverage Diff              @@
##               main    #3949      +/-   ##
============================================
- Coverage     70.54%   70.53%   -0.02%     
- Complexity    56798    56837      +39     
============================================
  Files          4573     4579       +6     
  Lines        273476   273768     +292     
  Branches      40101    40141      +40     
============================================
+ Hits         192925   193092     +167     
- Misses        64301    64368      +67     
- Partials      16250    16308      +58

Impacted Files	Coverage Δ
...er/src/main/java/org/opensearch/client/Client.java	`40.00% <ø> (ø)`
.../org/opensearch/client/support/AbstractClient.java	`34.78% <0.00%> (+0.07%)`	⬆️
...org/opensearch/action/search/DeletePitRequest.java	`21.27% <21.27%> (ø)`
.../opensearch/action/search/CreatePitController.java	`82.30% <62.96%> (-6.08%)`	⬇️
...main/java/org/opensearch/search/SearchService.java	`72.71% <76.66%> (+0.43%)`	⬆️
.../java/org/opensearch/action/search/PitService.java	`84.37% <84.37%> (ø)`
...rg/opensearch/action/search/DeletePitResponse.java	`93.54% <93.54%> (ø)`
...ensearch/action/search/SearchTransportService.java	`84.39% <96.55%> (-1.41%)`	⬇️
.../main/java/org/opensearch/action/ActionModule.java	`98.11% <100.00%> (+<0.01%)`	⬆️
.../org/opensearch/action/search/DeletePitAction.java	`100.00% <100.00%> (ø)`
... and 504 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

github-actions · 2022-07-21T05:56:59Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-07-21T07:00:38Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/863/
CommitID: 595afcd92328bd2ef58985c8856af08eaf19a797

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

github-actions · 2022-07-21T08:30:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/864/
CommitID: 717cd66

Bukhtawar · 2022-07-21T19:09:06Z

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

+                    .filter(r -> !r.isSuccessful())
+                    .forEach(r -> failedPitsStringBuilder.append(r.getPitId()).append(","));
+                logger.warn(() -> new ParameterizedMessage("Failed to delete PIT IDs {}", failedPitsStringBuilder.toString()));
+                if (!logger.isDebugEnabled()) return;


nit: put this inside the if block instead of negating it and returning early

Bukhtawar · 2022-07-21T19:15:18Z

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

+
+            @Override
+            public void onFailure(final Exception e) {
+                logger.error("Delete PITs failed", e);


Can we log more info on which PITs or how many failed

This onFailure is never called in our flow, during connection failures or during delete pit failures, we mark the associated pit ids 'isSucceeded' as false, which gives info that deletion failed.

This will only be executed when there is an unhandled exception and doubt we have context on pits at that point.

Bukhtawar · 2022-07-21T19:25:55Z

server/src/test/java/org/opensearch/search/DeletePitMultiNodeTests.java

+        client().admin().indices().prepareDelete("index1").get();
+    }
+
+    public void testDeleteWhileSearch() throws Exception {


Lets add more concurrent tests to ensure all failure cases around grouped listener is covered

github-actions · 2022-07-22T11:11:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/907/
CommitID: 31668b2bbfce2b44ffc279f84892eab1f387bbfb

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

github-actions · 2022-07-22T13:42:47Z

Gradle Check (Jenkins) Run Completed with:

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

github-actions · 2022-07-25T19:01:23Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/963/
CommitID: 38313f4db8cf8380163f128981b3fd0d9af456a2

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

github-actions · 2022-07-26T04:31:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/978/
CommitID: 3f08013

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

… into deletepitservice

github-actions · 2022-07-26T05:30:46Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/979/
CommitID: 61923493514304da08c8de580ffce26bbd5961d3

…ch into deletepitservice

github-actions · 2022-07-26T06:21:26Z

Gradle Check (Jenkins) Run Completed with:

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

bharath-techie added 15 commits July 15, 2022 17:02

Create pit service layer changes

a8be024

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

Create pit service layer changes

8a8f708

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

Delete pit service layer changes

f20dbdc

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

Create pit service layer changes

fa9946b

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

Merge branch 'createpitservice' of github.com:bharath-techie/OpenSear…

8f673c5

…ch into deletepitservice

changing to consistent action names

97c4f77

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

changing to consistent action names

bfcb50a

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

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>

Addressing review comments

987cb57

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

Merge branch 'createpitservice' of github.com:bharath-techie/OpenSear…

9497923

…ch into deletepitservice

Addressing review comments

8fcc25c

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

Merge branch 'createpitservice' of github.com:bharath-techie/OpenSear…

ab3f0d4

…ch into deletepitservice

Resolving conflicts

41d342d

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

bharath-techie force-pushed the deletepitservice branch from a309113 to 41d342d Compare July 19, 2022 15:18

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

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

sachinpkale reviewed Jul 20, 2022

View reviewed changes

Addressing comments

25bb645

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

dhruv16dhr mentioned this pull request Jul 20, 2022

[META] Point In Time Search Issue List #3959

Open

47 tasks

sachinpkale approved these changes Jul 20, 2022

View reviewed changes

bharath-techie changed the title ~~Delete PIT service layer changes~~ Implementing Delete PIT service layer changes Jul 20, 2022

bharath-techie force-pushed the deletepitservice branch from e36c60a to 595afcd Compare July 21, 2022 06:36

Addressing comments

717cd66

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

bharath-techie force-pushed the deletepitservice branch from 595afcd to 717cd66 Compare July 21, 2022 08:06

Bukhtawar reviewed Jul 21, 2022

View reviewed changes

Addressing review comment

f4db20b

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

bharath-techie force-pushed the deletepitservice branch from 31668b2 to f4db20b Compare July 22, 2022 13:19

Addressing review comment - adding tests to test concurrency

3f08013

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

bharath-techie force-pushed the deletepitservice branch from 38313f4 to 3f08013 Compare July 26, 2022 04:07

bharath-techie added 2 commits July 26, 2022 10:39

Addressing review comment - adding tests to test concurrency

f76fa24

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

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

d870dd5

… into deletepitservice

Merge branch 'deletepitservice' of github.com:bharath-techie/OpenSear…

ae618ff

…ch into deletepitservice

bharath-techie force-pushed the deletepitservice branch from 6192349 to ae618ff Compare July 26, 2022 05:57

Bukhtawar approved these changes Jul 26, 2022

View reviewed changes

Bukhtawar merged commit f4b0eef into opensearch-project:main Jul 26, 2022

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

reta mentioned this pull request Aug 2, 2022

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

Closed

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

shwetathareja mentioned this pull request Jun 19, 2025

Add @bharath-techie as maintainer #18556

Merged

Conversation

bharath-techie commented Jul 19, 2022

Description

Issues Resolved

Check List

Uh oh!

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Bukhtawar commented Jul 19, 2022

Uh oh!

sachinpkale commented Jul 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 20, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

codecov-commenter commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jul 21, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 21, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 21, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bukhtawar Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 22, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 22, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 25, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

github-actions bot commented Jul 26, 2022

Gradle Check (Jenkins) Run Completed with:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 20, 2022 •

edited

Loading

Bukhtawar Jul 21, 2022 •

edited

Loading