New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deletepit API #3

Open

bharath-techie wants to merge 34 commits into createpit from deletepit

Owner

bharath-techie commented Apr 18, 2022

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.

bharath-techie added 5 commits

April 13, 2022 09:56


          fixing ci, adding tests and java docs

34da6ea

Signed-off-by: Bharathwaj G <[email protected]>


          Segregating create pit logic into separate controller

30cb6c0

Signed-off-by: Bharathwaj G <[email protected]>


          Segregating create pit logic into separate controller

43fed74

Signed-off-by: Bharathwaj G <[email protected]>


          Delete PIT API

57232fb

Signed-off-by: Bharathwaj G <[email protected]>


          Delete PIT API

ece4c00

Signed-off-by: Bharathwaj G <[email protected]>

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 <[email protected]>

bharath-techie force-pushed the deletepit branch from 3e23f9d to d67e389 Compare

April 19, 2022 10:18

eirsep approved these changes

View reviewed changes

eirsep left a comment

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.

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

                   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]";

eirsep Apr 26, 2022

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]";

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

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

eirsep Apr 26, 2022

why is this ScrollFreeContextRequest

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

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

eirsep Apr 26, 2022

let's make it uniform and call it freeAllPitContexts maybe

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

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

eirsep Apr 26, 2022

why is this returning an empty transport response?

server/src/main/java/org/opensearch/action/search/TransportDeletePITAction.java Outdated Show resolved Hide resolved

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

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

eirsep Apr 26, 2022

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

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

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

eirsep Apr 26, 2022

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

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

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

eirsep Apr 26, 2022

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

Owner Author

bharath-techie May 3, 2022

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.

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

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

eirsep Apr 26, 2022

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?

Owner Author

bharath-techie May 3, 2022

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

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

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

eirsep Apr 26, 2022

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

eirsep suggested changes

View reviewed changes

eirsep left a comment

Meant to mark as request changes.

bharath-techie added 2 commits

April 29, 2022 17:37


          Addressing review comments

65c123a

Signed-off-by: Bharathwaj G <[email protected]>


          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…

… 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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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


          Bump com.diffplug.spotless from 6.5.2 to 6.6.1 (opensearch-project#3356)

87cb8a5

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] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>


          Bump grpc-context from 1.45.1 to 1.46.0 in /plugins/repository-gcs (o…

76b5ea6

…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] <[email protected]>

* Updating SHAs

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <dependabot[bot]@users.noreply.github.com>


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

892e984

…ensearch-project#3371)

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


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

f8b102c

…h-project#3364)

Signed-off-by: Ankit Jain <[email protected]>


          fixing tests and adding util

248f188

Signed-off-by: Bharathwaj G <[email protected]>

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 <[email protected]>


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

45a897f

…-project/OpenSearch into deletepit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet