Skip to content
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

Removed NOT_FOUND for empty results. #6544

Merged
merged 8 commits into from
Mar 9, 2023
Merged

Removed NOT_FOUND for empty results. #6544

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dc2ffc9
Removed NOT_FOUND for empty results.
Mar 4, 2023
2d2a705
Fixed failing tests.
Mar 5, 2023
bd33545
Removed unnecessary semi-colon.
Mar 5, 2023
f5ce3df
Added change log.
Mar 5, 2023
137cae6
Fixed formatting issues
Mar 5, 2023
8464bed
Backported to 2.x branch
Mar 6, 2023
b9fd886
Removed duplicate entry as this entry is present under [Unrelease 2.x].
Mar 6, 2023
26f1df5
Updated changelog to make it more meaningful.
Mar 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- 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))
- Add 'base_path' setting to File System Repository ([#6558](https://github.com/opensearch-project/OpenSearch/pull/6558))
- Return success on DeletePits when no PITs exist. ([#6544](https://github.com/opensearch-project/OpenSearch/pull/6544))

### Dependencies
- Bump `org.apache.logging.log4j:log4j-core` from 2.18.0 to 2.20.0 ([#6490](https://github.com/opensearch-project/OpenSearch/pull/6490))
Expand Down
6 changes: 4 additions & 2 deletions rest-api-spec/src/main/resources/rest-api-spec/test/pit/10_basic.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,5 +138,7 @@
- match: {pits.0.successful: true }

- do:
catch: missing
delete_all_pits: { }
delete_all_pits: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@harshjain2 harshjain2 Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"
		}
	]
}

Copy link
Contributor Author

@harshjain2 harshjain2 Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


- match: {pits: []}
- length: {pits: 0}
2 changes: 0 additions & 2 deletions server/src/main/java/org/opensearch/action/search/DeletePitResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import java.util.List;

import static org.opensearch.core.xcontent.ConstructingObjectParser.constructorArg;
import static org.opensearch.rest.RestStatus.NOT_FOUND;
import static org.opensearch.rest.RestStatus.OK;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return OK;
}

Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/opensearch/search/DeletePitResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public void testDeletePitResponseToXContent() throws IOException {
public void testDeletePitResponseToAndFromXContent() throws IOException {
XContentType xContentType = randomFrom(XContentType.values());
DeletePitResponse originalResponse = createDeletePitResponseTestItem();
;

BytesReference originalBytes = toShuffledXContent(originalResponse, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
DeletePitResponse parsedResponse;
try (XContentParser parser = createParser(xContentType.xContent(), originalBytes)) {
Expand Down