Deletepit API by bharath-techie · Pull Request #3 · bharath-techie/OpenSearch

bharath-techie · 2022-04-18T02:53:35Z

Description

Changes for delete pit api ( single, list and all )

Issues Resolved

[List any issues this PR will resolve]

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>

eirsep

Thanks for making these changes @bharath-techie.

Have comments on few code structuring

Skimmed through PR. Will take a deeper look and review along with test cases once rest tests, functional tests etc. are added.

eirsep · 2022-04-26T06:14:20Z

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

+    public static final String FREE_CONTEXT_PIT_ACTION_NAME = "indices:data/read/search[free_context/pit]";
    public static final String FREE_CONTEXT_ACTION_NAME = "indices:data/read/search[free_context]";
    public static final String CLEAR_SCROLL_CONTEXTS_ACTION_NAME = "indices:data/read/search[clear_scroll_contexts]";
+    public static final String DELETE_ALL_PIT_CONTEXTS_ACTION_NAME = "indices:data/read/search[delete_pit_contexts]";


variable name and action name should be consistent for delete all should be consistent with
public static final String FREE_CONTEXT_PIT_ACTION_NAME = "indices:data/read/search[free_context/pit]";

eirsep · 2022-04-26T06:14:49Z

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

+        transportService.sendRequest(
+            connection,
+            FREE_CONTEXT_PIT_ACTION_NAME,
+            new ScrollFreeContextRequest(contextId),


why is this ScrollFreeContextRequest

eirsep · 2022-04-26T06:15:40Z

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

        );
    }

+    public void sendDeleteAllPitContexts(Transport.Connection connection, final ActionListener<TransportResponse> listener) {


let's make it uniform and call it freeAllPitContexts maybe

eirsep · 2022-04-26T06:16:06Z

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

+            DELETE_ALL_PIT_CONTEXTS_ACTION_NAME,
+            TransportRequest.Empty.INSTANCE,
+            TransportRequestOptions.EMPTY,
+            new ActionListenerResponseHandler<>(listener, (in) -> TransportResponse.Empty.INSTANCE)


why is this returning an empty transport response?

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

eirsep · 2022-04-26T06:28:40Z

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

+            for (SearchContextIdForNode contextId : contexts) {
+                final DiscoveryNode node = nodeLookup.apply(contextId.getClusterAlias(), contextId.getNode());
+                if (node == null) {
+                    groupedListener.onFailure(new OpenSearchException("node not connected"));


"node not found" maybe? How do we know node even exists

eirsep · 2022-04-26T06:53:31Z

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

+    /**
+     * Delete all active PIT reader contexts
+     */
+    void deleteAllPits(ActionListener<DeletePITResponse> listener) {


can we move this to a separate handler class which would enable us to do better functional testing?

eirsep · 2022-04-26T06:53:41Z

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

+    /**
+     * Delete list of pits, return success if all reader contexts are deleted ( or not found ).
+     */
+    void deletePits(List<SearchContextIdForNode> contexts, ActionListener<Integer> listener) {


can we move this to a separate handler class which would enable us to do better functional testing?

unlike create pit (where mocking the transport action didn't give us the desired results), i was able to mock and write functional tests without segregating the methods into a separate file.

eirsep · 2022-04-26T06:54:34Z

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

+        }, listener::onFailure);
+    }
+
+    private StepListener<BiFunction<String, String, DiscoveryNode>> getLookupListener(List<SearchContextIdForNode> contexts) {


this seems like a reusable method between create/delete/pit or on those lines. redundant? Maybe all in one service class can harness common functionalities better?

not reusable since signature and hence logic is different between create and delete.

eirsep · 2022-04-26T06:55:22Z

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

+        return lookupListener;
+    }
+
+    private ActionListener<DeletePITResponse> getGroupedListener(ActionListener<DeletePITResponse> deletePitListener, int size) {


why a separate method for this if its used only once and is private hence not even being called in unit tests?

eirsep

Meant to mark as request changes.

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

… deletepit

… into createpit

… deletepit

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

… deletepit

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

… deletepit

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

… deletepit

Bumps com.diffplug.spotless from 6.5.2 to 6.6.1. --- updated-dependencies: - dependency-name: com.diffplug.spotless dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…pensearch-project#3361) * Bump grpc-context from 1.45.1 to 1.46.0 in /plugins/repository-gcs Bumps [grpc-context](https://github.com/grpc/grpc-java) from 1.45.1 to 1.46.0. - [Release notes](https://github.com/grpc/grpc-java/releases) - [Commits](grpc/grpc-java@v1.45.1...v1.46.0) --- updated-dependencies: - dependency-name: io.grpc:grpc-context dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Updating SHAs Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>

…ensearch-project#3371) Signed-off-by: Suraj Singh <surajrider@gmail.com>

…h-project#3364) Signed-off-by: Ankit Jain <jain.ankitk@gmail.com>

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

… deletepit

* Create Point In Time API changes Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

…-project/OpenSearch into deletepit

bharath-techie added 5 commits April 13, 2022 09:56

fixing ci, adding tests and java docs

34da6ea

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

Segregating create pit logic into separate controller

30cb6c0

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

Segregating create pit logic into separate controller

43fed74

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

Delete PIT API

57232fb

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

Delete PIT API

ece4c00

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

bharath-techie force-pushed the deletepit branch 3 times, most recently from 8f23568 to 3e23f9d Compare April 19, 2022 09:19

Delete PIT API changes

d67e389

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

bharath-techie force-pushed the deletepit branch from 3e23f9d to d67e389 Compare April 19, 2022 10:18

eirsep approved these changes Apr 26, 2022

View reviewed changes

eirsep suggested changes Apr 26, 2022

View reviewed changes

bharath-techie added 2 commits April 29, 2022 17:37

Addressing review comments

65c123a

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

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

a938e26

… deletepit

bharath-techie force-pushed the createpit branch from 65c123a to 3359986 Compare May 2, 2022 09:56

bharath-techie added 2 commits May 2, 2022 15:34

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

6789468

… into createpit

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

98451da

… into createpit

bharath-techie force-pushed the createpit branch from 6789468 to 3448387 Compare May 2, 2022 11:17

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

975b871

… into createpit

bharath-techie force-pushed the createpit branch from 3448387 to 975b871 Compare May 2, 2022 11:31

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

2a8e4ff

… deletepit

bharath-techie changed the base branch from createpit to main May 2, 2022 12:56

bharath-techie changed the base branch from main to createpit May 2, 2022 12:56

addressing review comments

77be351

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

bharath-techie force-pushed the createpit branch from 975b871 to 232179e Compare May 2, 2022 13:58

bharath-techie added 2 commits May 2, 2022 22:34

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

849d1d3

… deletepit

addressing review comments

51ce82f

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

bharath-techie force-pushed the deletepit branch from 4120c50 to fa1f2cd Compare May 4, 2022 04:25

bharath-techie force-pushed the createpit branch 2 times, most recently from 6cda616 to 730afa8 Compare May 5, 2022 08:12

bharath-techie force-pushed the deletepit branch from 7c408d5 to 480bdc2 Compare May 11, 2022 06:49

adding tests and comments

04532f4

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

bharath-techie force-pushed the createpit branch 5 times, most recently from 7a77f7b to 929800d Compare May 13, 2022 07:48

addressing comments

6ceaf61

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

bharath-techie force-pushed the createpit branch from 929800d to 6ceaf61 Compare May 13, 2022 07:58

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

fe4e41a

… deletepit

bharath-techie force-pushed the createpit branch 3 times, most recently from f65aa43 to 35d8cc4 Compare May 13, 2022 10:57

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

1dd62b4

… deletepit

bharath-techie force-pushed the deletepit branch from 8e4d008 to 1dd62b4 Compare May 17, 2022 05:15

Rest high level clients

af9b143

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

bharath-techie force-pushed the deletepit branch from d19c2d3 to af9b143 Compare May 17, 2022 14:27

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

dc111f5

… deletepit

bharath-techie force-pushed the deletepit branch 3 times, most recently from 100e1ea to d2e972b Compare May 18, 2022 17:23

dependabot bot and others added 5 commits May 18, 2022 10:31

[Type removal] Remove redundant _type in pipeline simulate action (op…

892e984

…ensearch-project#3371) Signed-off-by: Suraj Singh <surajrider@gmail.com>

Removing hard coded value of max concurrent shard requests (opensearc…

f8b102c

…h-project#3364) Signed-off-by: Ankit Jain <jain.ankitk@gmail.com>

fixing tests and adding util

248f188

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

bharath-techie force-pushed the deletepit branch from d2e972b to 248f188 Compare May 18, 2022 18:26

bharath-techie and others added 3 commits May 19, 2022 10:46

Merge branch 'createpit' of github.com:bharath-techie/OpenSearch into…

829d79e

… deletepit

Create PIT API (opensearch-project#2745)

4a1cbd9

* Create Point In Time API changes Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

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

45a897f

…-project/OpenSearch into deletepit

Conversation

bharath-techie commented Apr 18, 2022

Description

Issues Resolved

Check List

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!

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!

eirsep left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants