Removed NOT_FOUND for empty results. #6544

harshjain2 · 2023-03-05T15:40:44Z

Description

Fix for the issue due to which 404 was returned when no PITs were present.

Issues Resolved

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
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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 · 2023-03-05T16:01:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11978/
CommitID: e9f604f
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

reta · 2023-03-05T16:29:18Z

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

@@ -57,7 +56,6 @@ public List<DeletePitInfo> getDeletePitResults() {
     */
    @Override
    public RestStatus status() {
-        if (deletePitResults.isEmpty()) return NOT_FOUND;


So what would be the payload returned from the method? Empty list of PITs?

The server returns 200 StatusCode with { "pits": [] } as the body when there are no PITs to delete.

reta

Please add the test cases

harshjain2 · 2023-03-05T17:30:33Z

@reta I have fixed integration tests. By this comment, do you mean unit tests?

reta · 2023-03-05T17:33:38Z

@reta I have fixed integration tests. By this comment, do you mean unit tests?

The rest-api-spec is fine, thank you

reta · 2023-03-05T17:34:01Z

Also, please fix DCO check, it is failing

github-actions · 2023-03-05T17:43:55Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11979/
CommitID: 2503566
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

harshjain2 · 2023-03-06T17:11:13Z

Wht is the criteria to merge this PR. All the feedback providers must approve this PR?

reta · 2023-03-06T17:17:43Z

Wht is the criteria to merge this PR. All the feedback providers must approve this PR?

Yes

dreamer-89 · 2023-03-06T17:19:45Z

@reta @harshjain2 : Anything pending for merge ?

harshjain2 · 2023-03-06T17:23:16Z

@dreamer-89 @reta I have addressed all the feedback provided on the PR. Nothing pending from my side on this PR.

reta · 2023-03-06T17:31:26Z

@dreamer-89 @reta I have addressed all the feedback provided on the PR. Nothing pending from my side on this PR.

I also approved, @Arpit-Bandejiya your concerns were cleared out?

andrross · 2023-03-07T17:45:43Z

CHANGELOG.md

@@ -73,6 +73,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Add GeoTile and GeoHash Grid aggregations on GeoShapes. ([#5589](https://github.com/opensearch-project/OpenSearch/pull/5589))
 - Disallow multiple data paths for search nodes ([#6427](https://github.com/opensearch-project/OpenSearch/pull/6427))
 - [Segment Replication] Allocation and rebalancing based on average primary shard count per index ([#6422](https://github.com/opensearch-project/OpenSearch/pull/6422))
+- Removed NOT_FOUND for empty results. ([#6544](https://github.com/opensearch-project/OpenSearch/pull/6544))


Check out the contributing doc for more details, but this entry isn't super helpful as a release note because it doesn't provide any context about what this change means. I'd suggest something like "Return success on DeletePits when nothing is deleted" or "Return success on DeletePits when no PITs exist".

andrross · 2023-03-07T17:47:14Z

rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml

@@ -138,5 +138,7 @@
  - match: {pits.0.successful: true }

  - do:
-      catch: missing
-      delete_all_pits: { }
+      delete_all_pits: {}


Does this change the behavior of deleting a single PIT or a list of PITs? Should there be a test for those cases as well?

@andrross I don't think it will result in any change in the behaviour of deleting a single PIT or a list of PITs using DELETE localhost:9200/_search/point_in_time/ API.

Request Delete PIT with empty array results in validation exception.

DELETE localhost:9200/_search/point_in_time/
Request Body

{ "pit_id": [] }

Response

{ "error": { "root_cause": [ { "type": "action_request_validation_exception", "reason": "Validation Failed: 1: no pit ids specified;" } ], "type": "action_request_validation_exception", "reason": "Validation Failed: 1: no pit ids specified;" }, "status": 400 }

Request with incorrect pits results in failure with failure message associated with every pit id

DELETE localhost:9200/_search/point_in_time/
Request Body

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

Response

{ "pits": [ { "successful": false, "pit_id": "o463QQEPbXktaW5kZXgtMDAwMDAxFkhGN09fMVlPUkVPLXh6MUExZ1hpaEEAFjBGbmVEZHdGU1EtaFhhUFc4ZkR5cWcAAAAAAAAAAAEWaXBPNVJtZEhTZDZXTWFFR05waXdWZwEWSEY3T18xWU9SRU8teHoxQTFnWGloQQAA" }, { "successful": false, "pit_id": "o463QQEPbXktaW5kZXgtMDAwMDAxFkhGN09fMVlPUkVPLXh6MUExZ1hpaEEAFjBGbmVEZHdGU1EtaFhhUFc4ZkR5cWcAAAAAAAAAAAIWaXBPNVJtZEhTZDZXTWFFR05waXdWZwEWSEY3T18xWU9SRU8teHoxQTFnWGloQQAA" } ] }

@andrross Should there be a test for those cases as well?
Few tests cases already exist for the DELETE PIT(s). But I couldn't find test case for the use case where empty pit array is passed in the body.

DELETE localhost:9200/_search/point_in_time/ Request Body: { "pit_id": [] }

I believe this should be tracked through a separate issue.

github-actions · 2023-03-08T07:13:06Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12103/
CommitID: bc8974e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-08T08:26:30Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/12105/
CommitID: 133cc33

andrross · 2023-03-09T02:15:18Z

@harshjain2 Can you rebase and fix the conflicts?

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Signed-off-by: Harsh Jain <harshjai@amazon.com> Signed-off-by: Harsh Jain <harshjai@amazon.com>

Signed-off-by: Harsh Jain <harshjai@amazon.com>

github-actions · 2023-03-09T06:07:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/12158/
CommitID: 26f1df5

opensearch-trigger-bot · 2023-03-09T13:01:38Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-6544-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 67a9b439db79f2782a3fd071004e3ff02c59ba13
# Push it to GitHub
git push --set-upstream origin backport/backport-6544-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6544-to-2.x.

reta · 2023-03-09T13:04:17Z

@harshjain2 the backport to 2.x failed :( could you please do a manual one? thank you

harshjain2 · 2023-03-10T07:08:12Z

Backport PR to 2.x : #6614

* Removed NOT_FOUND for empty results. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Fixed failing tests. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Removed unnecessary semi-colon. Signed-off-by: Harsh Jain <harshjai@amazon.com> Signed-off-by: Harsh Jain <harshjai@amazon.com> * Added change log. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Fixed formatting issues Signed-off-by: Harsh Jain <harshjai@amazon.com> * Backported to 2.x branch Signed-off-by: Harsh Jain <harshjai@amazon.com> * Removed duplicate entry as this entry is present under [Unrelease 2.x]. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Updated changelog to make it more meaningful. Signed-off-by: Harsh Jain <harshjai@amazon.com> --------- Signed-off-by: Harsh Jain <harshjai@amazon.com> Co-authored-by: Harsh Jain <harshjai@amazon.com> (cherry picked from commit 67a9b43)

* Removed NOT_FOUND for empty results. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Fixed failing tests. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Removed unnecessary semi-colon. Signed-off-by: Harsh Jain <harshjai@amazon.com> Signed-off-by: Harsh Jain <harshjai@amazon.com> * Added change log. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Fixed formatting issues Signed-off-by: Harsh Jain <harshjai@amazon.com> * Backported to 2.x branch Signed-off-by: Harsh Jain <harshjai@amazon.com> * Removed duplicate entry as this entry is present under [Unrelease 2.x]. Signed-off-by: Harsh Jain <harshjai@amazon.com> * Updated changelog to make it more meaningful. Signed-off-by: Harsh Jain <harshjai@amazon.com> --------- Signed-off-by: Harsh Jain <harshjai@amazon.com> Co-authored-by: Harsh Jain <harshjai@amazon.com> Signed-off-by: Mingshi Liu <mingshl@amazon.com>

harshjain2 mentioned this pull request Mar 5, 2023

[BUG] Delete All PITs returns 404 if no PITs are present #5938

Closed

reta reviewed Mar 5, 2023

View reviewed changes

reta requested changes Mar 5, 2023

View reviewed changes

dreamer-89 approved these changes Mar 6, 2023

View reviewed changes

andrross requested changes Mar 7, 2023

View reviewed changes

andrross approved these changes Mar 9, 2023

View reviewed changes

Harsh Jain added 8 commits March 9, 2023 11:08

Removed NOT_FOUND for empty results.

dc2ffc9

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Fixed failing tests.

2d2a705

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Removed unnecessary semi-colon.

bd33545

Signed-off-by: Harsh Jain <harshjai@amazon.com> Signed-off-by: Harsh Jain <harshjai@amazon.com>

Added change log.

f5ce3df

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Fixed formatting issues

137cae6

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Backported to 2.x branch

8464bed

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Removed duplicate entry as this entry is present under [Unrelease 2.x].

b9fd886

Signed-off-by: Harsh Jain <harshjai@amazon.com>

Updated changelog to make it more meaningful.

26f1df5

Signed-off-by: Harsh Jain <harshjai@amazon.com>

reta merged commit 67a9b43 into opensearch-project:main Mar 9, 2023

reta added bug Something isn't working backport 2.x Backport to 2.x branch labels Mar 9, 2023

reta mentioned this pull request Mar 9, 2023

[Backport] [2.x] OpenSSLTest is not using the OpenSSL Provider (#2301) opensearch-project/security#2533

Merged

harshjain2 mentioned this pull request Mar 10, 2023

[Backport] [2.x] Removed NOT_FOUND for empty results. (#6544) #6614

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed NOT_FOUND for empty results. #6544

Removed NOT_FOUND for empty results. #6544

harshjain2 commented Mar 5, 2023 •

edited

github-actions bot commented Mar 5, 2023

reta Mar 5, 2023

harshjain2 Mar 5, 2023

reta left a comment

harshjain2 commented Mar 5, 2023

reta commented Mar 5, 2023

reta commented Mar 5, 2023

github-actions bot commented Mar 5, 2023

harshjain2 commented Mar 6, 2023

reta commented Mar 6, 2023

dreamer-89 commented Mar 6, 2023

harshjain2 commented Mar 6, 2023

reta commented Mar 6, 2023

andrross Mar 7, 2023 •

edited

harshjain2 Mar 8, 2023

andrross Mar 7, 2023

harshjain2 Mar 8, 2023 •

edited

harshjain2 Mar 8, 2023 •

edited

github-actions bot commented Mar 8, 2023

github-actions bot commented Mar 8, 2023

andrross commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

opensearch-trigger-bot bot commented Mar 9, 2023

reta commented Mar 9, 2023

harshjain2 commented Mar 10, 2023

Removed NOT_FOUND for empty results. #6544

Removed NOT_FOUND for empty results. #6544

Conversation

harshjain2 commented Mar 5, 2023 • edited

Description

Issues Resolved

Check List

github-actions bot commented Mar 5, 2023

Gradle Check (Jenkins) Run Completed with:

reta Mar 5, 2023

Choose a reason for hiding this comment

harshjain2 Mar 5, 2023

Choose a reason for hiding this comment

reta left a comment

Choose a reason for hiding this comment

harshjain2 commented Mar 5, 2023

reta commented Mar 5, 2023

reta commented Mar 5, 2023

github-actions bot commented Mar 5, 2023

Gradle Check (Jenkins) Run Completed with:

harshjain2 commented Mar 6, 2023

reta commented Mar 6, 2023

dreamer-89 commented Mar 6, 2023

harshjain2 commented Mar 6, 2023

reta commented Mar 6, 2023

andrross Mar 7, 2023 • edited

Choose a reason for hiding this comment

harshjain2 Mar 8, 2023

Choose a reason for hiding this comment

andrross Mar 7, 2023

Choose a reason for hiding this comment

harshjain2 Mar 8, 2023 • edited

Choose a reason for hiding this comment

Request Delete PIT with empty array results in validation exception.

Request with incorrect pits results in failure with failure message associated with every pit id

harshjain2 Mar 8, 2023 • edited

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 8, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Mar 9, 2023

github-actions bot commented Mar 9, 2023

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Mar 9, 2023

reta commented Mar 9, 2023

harshjain2 commented Mar 10, 2023

harshjain2 commented Mar 5, 2023 •

edited

andrross Mar 7, 2023 •

edited

harshjain2 Mar 8, 2023 •

edited

harshjain2 Mar 8, 2023 •

edited