Delete PIT changes by bharath-techie · Pull Request #3401 · opensearch-project/OpenSearch

bharath-techie · 2022-05-19T06:59:11Z

Description

This PR contains Delete PIT API changes. User can use '_all' to delete all pit contexts, or pass list of pit IDs as part of body to delete PITs.

Create PIT PR : #2745

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.

opensearch-ci-bot · 2022-05-19T07:13:55Z

❌ Gradle Check failure 20520b22af5cdee3a7d8daad50ef088a501eca5f
Log 5472

Reports 5472

opensearch-ci-bot · 2022-05-19T07:14:59Z

❌ Gradle Check failure 0b442923704bb5e32c317ca97777f9fa677f77f3
Log 5473

Reports 5473

opensearch-ci-bot · 2022-05-19T07:29:26Z

❌ Gradle Check failure 9f382e7e2ec70f681a76f642e8f697c955bbd173
Log 5474

Reports 5474

opensearch-ci-bot · 2022-05-19T09:23:25Z

✅ Gradle Check success 53ee52c3e77d01232ecd2a021cc83c87e21cea6b
Log 5475

Reports 5475

opensearch-ci-bot · 2022-05-19T11:56:32Z

❌ Gradle Check failure 8229655db3ae8ccacbf564a691aaa33899f5822d
Log 5477

Reports 5477

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

opensearch-ci-bot · 2022-05-19T14:49:50Z

✅ Gradle Check success 25cc2b1
Log 5483

Reports 5483

bharath-techie · 2022-05-19T17:01:15Z

@nknize , @andrross please take a look at this PR.

andrross · 2022-05-19T20:15:39Z

Regarding the API, #1147 describes DELETE /_point_in_time/<id> whereas this implements no ID in the URL and instead an array of IDs in the body. What is the reason for the change? I don't have enough knowledge of the typical use cases to have a strong opinion, other than to say that a single resource delete is simpler. If we go with the array approach, then we'll need to clearly define (and test for) the behavior when some of the provided IDs are valid and some are not valid. That problem is avoided with the simpler delete API.

bharath-techie · 2022-05-20T02:43:49Z

Regarding the API, #1147 describes DELETE /_point_in_time/<id> whereas this implements no ID in the URL and instead an array of IDs in the body. What is the reason for the change? I don't have enough knowledge of the typical use cases to have a strong opinion, other than to say that a single resource delete is simpler. If we go with the array approach, then we'll need to clearly define (and test for) the behavior when some of the provided IDs are valid and some are not valid. That problem is avoided with the simpler delete API.

Since PIT ID is quite large, and we are supporting deletion of list of pit ids, we made this change to support only list of pit ids in body.

Similar APIs like clear scroll also forms a similar pattern. ( though it supports passing id as part of params, now its deprecated )

Behavior:
The current implementation as part of this PR returns 'succeeded' as 'true' if all PITS are successfully deleted or 'not found'. Only for cases like node drop, or actual PIT deletion failure (where PIT is present but we're not able to delete), we will mark the operation as unsuccessful and 'succeeded' as 'false'.

Invalid IDs:
For non decodable ids, we throw proper validation errors stating that id is not valid. For ids expired, we simply mark the operation as succeeded.

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

opensearch-ci-bot · 2022-05-20T03:32:58Z

✅ Gradle Check success 724dffc
Log 5499

Reports 5499

eirsep · 2022-05-20T06:28:39Z

client/rest-high-level/src/test/java/org/opensearch/client/SearchIT.java

+                highLevelClient()::deletePit,
+                highLevelClient()::deletePitAsync
+            );
+            assertTrue(deletePitResponse.isSucceeded());


let's verify that searchservice map of activeReader is empty on each node

we can use 'list all pits api' to verify active pits as part of 'list all pits' pr.

eirsep

Thanks for making this changes Bharat.

I have requested some

changes in Delete PIT logic
suggested an optimization for delete PIt contexts to reduce transport calls

Apart from that have a few minor comments.
Some points to ponder over:

should we rename from delete PIT to clear PIT. that would in keeping with how scroll is named.
we could avoid using the term reader context in documentation and rest layer as it might only confuse people. That's a transport layer or service layer construct. As a user I will be only worried if PIT is deleted or not. I wouldn't be wanting to know fi contexts are cleared or not. Just if my operation succeeded.

Will review tests after this batch of comments are addressed

eirsep · 2022-05-20T06:31:34Z

rest-api-spec/src/main/resources/rest-api-spec/api/delete_pit.json

+  "delete_pit":{
+    "documentation":{
+      "url":"https://opensearch.org/docs/latest/opensearch/rest-api/point_in_time/",
+      "description":"Deletes one or more point in time contexts."


Can be worded differently. Maybe something more on the lines of "Deletes all point-in-time whose id is passed"?

Deletes one or more point in time contexts sounds confusing
User wants an api to delete a PIT. Contexts might be confusing.

eirsep · 2022-05-20T06:32:02Z

rest-api-spec/src/main/resources/rest-api-spec/api/delete_all_pits.json

+  "delete_all_pits":{
+    "documentation":{
+      "url":"https://opensearch.org/docs/latest/opensearch/rest-api/point_in_time/",
+      "description":"Deletes all active point in time contexts."


should we mention "contexts"?

context is used as part of scroll as well - 'Explicitly clears the search context for a scroll.'

i think just saying point of time will be confusing, we can take a second opinion here.

eirsep · 2022-05-20T06:34:24Z

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

+import org.opensearch.action.ActionType;
+
+/**
+ * Action type for deleting PIT reader contexts


same as above. let's talk about deleting point-in-time's

the fact that PIT contexts are deleted could be an abstraction to the rest and transport layer.

This description seems to hint we can deleted contexts selectively.

let's talk about deleting point-in-time's

I would consider using "point-in-time search" as the noun form to disambiguate from future point-in-times (e.g. point-in-time restore). It also aligns with the _search/point_in_time resource path.

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

eirsep · 2022-05-20T06:40:44Z

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

+        pitIds = Arrays.asList(in.readStringArray());
+    }
+
+    public DeletePitRequest(String... pitIds) {


why is this required?

public DeletePitRequest(List<String> pitIds) should suffice right?

As we use "_all" , this will be needed.

eirsep · 2022-05-20T07:23:46Z

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

+import java.util.List;
+
+/**
+ * Transport action for deleting pit reader context - supports deleting list and all pit contexts


we are deleting a point in time. Let's not say "deleting pit reader context".

Supports deletion of a list of PIT by their Id's or all PITs

eirsep · 2022-05-20T07:25:27Z

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

 /**
 * Helper class for common search functions
 */
 public class SearchUtils {


still not sure why deletePitContexts is not present in PITController and why CreatePitController is not just called PitController or PitService catering to all things PIT

Not a big fan of Utils with static method, any reason why we can't have them encapsulated in a service

Made the change to make it a separate service. One challenge with just putting everything in create pit controller is that there are too many dependencies and integ tests fail with inject errors ( sometimes even on the classes which are existing ). So created a new lightweight service file which will have the common methods. Create pit controller will continue to cater create pit specific methods.

eirsep · 2022-05-20T07:26:20Z

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

+    @Override
+    protected void doExecute(Task task, DeletePitRequest request, ActionListener<DeletePitResponse> listener) {
+        List<String> pitIds = request.getPitIds();
+        if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) {


what happens if some one passes PIT Id1, PIT Id2, _all i.e. size!=1 but contains _all ?

we will throw exception when '_all' is passed as part of body as its not a valid encoded pit id. Current implementation accepts '_all' as part of params as its in line with other similar impl such as clear scrolls.

eirsep · 2022-05-20T07:28:43Z

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

+                listener.onResponse(new DeletePitResponse(false));
+            }
+        }, e -> {
+            logger.debug("Delete PITs failed ", e);


this is an ERROR level log

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

andrross · 2022-05-20T17:07:15Z

re: Behavior:

What happens if there is one invalid ID in a list of otherwise valid IDs?
What happens if there is real failure when deleting from a list of IDs? Will the API return "not successful" even though some IDs were deleted?
What happens if "_all" is passed as an ID in the body of DELETE /_search/point_in_time?
Should there be an upper bound in the number of IDs that can be passed in DELETE /_search/point_in_time?

As an example, take a look at the documentation for the DynamoDB BatchWriteItem API. It clearly defines how partial success is handled and cases where the entire batch will be rejected. We'll want to think through all those same cases here.

andrross · 2022-05-20T17:13:52Z

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

+    }
+
+    public DeletePitRequest(List<String> pitIds) {
+        if (pitIds != null) {


Is null really valid input here? I'd consider just letting this fail with an NPE (or throwing an IllegalArgumentException).

andrross · 2022-05-20T17:17:19Z

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

+        }
+    }
+
+    public void addPitId(String pitId) {


Do you need to expose this as a public method? I'd consider simplifying this class by:

remove this method

make pitIds final, and assign it to an empty list in the default constructor case

in fromXContent clear pidIds and then add to it directly instead of calling this method

andrross · 2022-05-20T17:18:37Z

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

+
+    public DeletePitRequest(List<String> pitIds) {
+        if (pitIds != null) {
+            this.pitIds = pitIds;


Should we make a defensive copy of the list in this case? This is vulnerable to the action-at-a-distance problem if the caller later modifies the list that was passed here.

server/src/main/java/org/opensearch/rest/action/search/RestDeletePitAction.java

opensearch-ci-bot · 2022-05-24T09:39:38Z

❌ Gradle Check failure 2219aebc9219391ad9f38b80a56560b04b8f56a0
Log 5559

Reports 5559

opensearch-ci-bot · 2022-05-24T11:58:50Z

❌ Gradle Check failure 0ffc780b8e21378b4fe470bc4ea44f8e31c33b77
Log 5561

Reports 5561

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

opensearch-ci-bot · 2022-05-24T13:48:38Z

❌ Gradle Check failure e482053
Log 5564

Reports 5564

opensearch-ci-bot · 2022-06-24T07:22:23Z

❌ Gradle Check failure 36dcd86b4348ea030271053abe31388f7418f439
Log 6291

Reports 6291

opensearch-ci-bot · 2022-06-24T07:42:09Z

❌ Gradle Check failure a569b07903830a8e764d9a40fa61734f79f34d8e
Log 6292

Reports 6292

opensearch-ci-bot · 2022-06-25T07:46:02Z

❌ Gradle Check failure f9db591aec235045f91c909589e1bf069c83b64d
Log 6334

Reports 6334

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

opensearch-ci-bot · 2022-06-27T05:54:41Z

❌ Gradle Check failure 93f8db7
Log 6354

Reports 6354

…-project/OpenSearch into delete_pit

opensearch-ci-bot · 2022-06-27T09:24:41Z

❌ Gradle Check failure 17a824e
Log 6360

Reports 6360

opensearch-ci-bot · 2022-06-27T10:38:26Z

❌ Gradle Check failure 2e4c56c4118d1d1c902a2bce6cb440b4ccb1fd65
Log 6361

Reports 6361

opensearch-ci-bot · 2022-06-27T12:35:37Z

❌ Gradle Check failure 758aaaca96d6507390904a8648c31840df1bcae4
Log 6362

Reports 6362

opensearch-ci-bot · 2022-06-27T13:28:22Z

❌ Gradle Check failure 2a25a8a099c7a42131f6146ab0ebffe8d26abbaa
Log 6365

Reports 6365

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

opensearch-ci-bot · 2022-06-27T14:15:56Z

✅ Gradle Check success d4ecca2
Log 6369

Reports 6369

sachinpkale · 2022-06-27T17:20:45Z

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

        // and create pit request keep alive is less than that, keep alive is set to 1 second, (max of 2 keep alives)
        // so reader context will clear up after 1 second
-        Thread.sleep(1000);
+        Thread.sleep(1200);


Thread.sleep() calls make a test flaky. As everything depends on the state of the server at the time tests are running and CPU context switch that is happening, the code that we expect to finish in 1200ms may take more than that. Is there any way to provide listener?

Bukhtawar · 2022-06-23T09:42:17Z

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

+                if (!logger.isDebugEnabled()) return;
+                for (DeletePitInfo deletePitInfo : response.getDeletePitResults()) {
+                    if (!deletePitInfo.isSucceeded()) {
+                        logger.debug(() -> new ParameterizedMessage("Failed to delete PIT ID {}", deletePitInfo.getPitId()));


Why debug, should this be warn instead?. Lets have a single log line with concatenated ids?

Bukhtawar · 2022-06-23T09:43:07Z

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

+    /**
+     * This will be true if PIT reader contexts are deleted ond also if contexts are not found.
+     */
+    private final boolean succeeded;


Rename to successful

Bukhtawar · 2022-06-23T09:46:34Z

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

+    @Override
+    public ActionRequestValidationException validate() {
+        ActionRequestValidationException validationException = null;
+        if (pitIds == null || pitIds.isEmpty()) {


Use collection utility?

collection utils works on arrays , since this is list, keeping the checks same

Bukhtawar · 2022-06-28T16:21:27Z

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

 /**
 * Helper class for common search functions
 */
 public class SearchUtils {


Not a big fan of Utils with static method, any reason why we can't have them encapsulated in a service

Bukhtawar · 2022-06-28T16:22:06Z

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

+    private void deletePits(ActionListener<DeletePitResponse> listener, DeletePitRequest request) {
+        Map<String, List<PitSearchContextIdForNode>> nodeToContextsMap = new HashMap<>();
+        for (String pitId : request.getPitIds()) {
+            SearchContextId contextId = SearchContextId.decode(namedWriteableRegistry, pitId);
+            for (SearchContextIdForNode contextIdForNode : contextId.shards().values()) {
+                PitSearchContextIdForNode pitSearchContext = new PitSearchContextIdForNode(pitId, contextIdForNode);
+                List<PitSearchContextIdForNode> contexts = nodeToContextsMap.getOrDefault(contextIdForNode.getNode(), new ArrayList<>());
+                contexts.add(pitSearchContext);
+                nodeToContextsMap.put(contextIdForNode.getNode(), contexts);
+            }
+        }
+        SearchUtils.deletePitContexts(nodeToContextsMap, listener, clusterService.state(), searchTransportService);
+    }
+


Lets move this to a Service

Made the change to make it a separate service. One challenge with just putting everything in create pit controller is that there are too many dependencies and integ tests fail with inject errors ( sometimes even on the classes which are existing ). So created a new lightweight service file which will have the common methods. Create pit controller will continue to cater create pit specific methods.

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

* Delete PIT changes Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie requested review from a team and reta as code owners May 19, 2022 06:59

bharath-techie force-pushed the delete_pit branch from 20520b2 to 0b44292 Compare May 19, 2022 07:02

bharath-techie force-pushed the delete_pit branch from 0b44292 to 9f382e7 Compare May 19, 2022 07:21

bharath-techie force-pushed the delete_pit branch from 9f382e7 to 53ee52c Compare May 19, 2022 08:48

bharath-techie force-pushed the delete_pit branch from 53ee52c to 8229655 Compare May 19, 2022 11:42

Delete PIT changes

25cc2b1

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

bharath-techie force-pushed the delete_pit branch from 8229655 to 25cc2b1 Compare May 19, 2022 14:12

Delete PIT changes - adding tests for invalid ids

724dffc

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

eirsep reviewed May 20, 2022

View reviewed changes

eirsep suggested changes May 20, 2022

View reviewed changes

andrross reviewed May 20, 2022

View reviewed changes

bharath-techie force-pushed the delete_pit branch 2 times, most recently from 9a4f707 to 0ffc780 Compare May 24, 2022 11:33

Addressing comments

e482053

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

bharath-techie force-pushed the delete_pit branch from 0ffc780 to e482053 Compare May 24, 2022 13:30

bharath-techie force-pushed the delete_pit branch from a569b07 to f9db591 Compare June 25, 2022 07:33

Addressing comments - adding tests

93f8db7

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

bharath-techie force-pushed the delete_pit branch from f9db591 to 93f8db7 Compare June 27, 2022 05:36

Merge branch 'feature/point_in_time' of https://github.com/opensearch…

17a824e

…-project/OpenSearch into delete_pit

bharath-techie force-pushed the delete_pit branch from 2e4c56c to 758aaac Compare June 27, 2022 11:56

bharath-techie force-pushed the delete_pit branch from 758aaac to 2a25a8a Compare June 27, 2022 12:56

Fixing gradle errors

d4ecca2

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

bharath-techie force-pushed the delete_pit branch from 2a25a8a to d4ecca2 Compare June 27, 2022 13:44

sachinpkale reviewed Jun 27, 2022

View reviewed changes

sachinpkale approved these changes Jun 28, 2022

View reviewed changes

Bukhtawar reviewed Jun 28, 2022

View reviewed changes

bharath-techie force-pushed the delete_pit branch 2 times, most recently from 5d62ef1 to 17419e8 Compare June 29, 2022 09:21

Addressing comments

beacfc1

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

bharath-techie force-pushed the delete_pit branch from 17419e8 to beacfc1 Compare June 29, 2022 10:13

Bukhtawar approved these changes Jul 5, 2022

View reviewed changes

Bukhtawar merged commit ad282d7 into opensearch-project:feature/point_in_time Jul 5, 2022

bharath-techie added a commit to bharath-techie/OpenSearch that referenced this pull request Jul 14, 2022

Delete PIT changes (opensearch-project#3401)

f05b7bf

* Delete PIT changes Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie mentioned this pull request Jul 19, 2022

Implementing Delete PIT service layer changes #3949

Merged

5 tasks

dhruv16dhr mentioned this pull request Jul 20, 2022

[META] Point In Time Search Issue List #3959

Open

47 tasks

Conversation

bharath-techie commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented May 19, 2022

Uh oh!

opensearch-ci-bot commented May 19, 2022

Uh oh!

opensearch-ci-bot commented May 19, 2022

Uh oh!

opensearch-ci-bot commented May 19, 2022

Uh oh!

opensearch-ci-bot commented May 19, 2022

Uh oh!

opensearch-ci-bot commented May 19, 2022

Uh oh!

bharath-techie commented May 19, 2022

Uh oh!

andrross commented May 19, 2022

Uh oh!

bharath-techie commented May 20, 2022

Uh oh!

opensearch-ci-bot commented May 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eirsep left a comment

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!

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!

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!

Uh oh!

andrross commented May 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!

Uh oh!

opensearch-ci-bot commented May 24, 2022

Uh oh!

opensearch-ci-bot commented May 24, 2022

Uh oh!

opensearch-ci-bot commented May 24, 2022

bharath-techie commented May 19, 2022 •

edited

Loading