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

[Remove] types from SearchHit and Explain API #2205

Merged
merged 6 commits into from
Feb 23, 2022
Merged

[Remove] types from SearchHit and Explain API #2205

merged 6 commits into from
Feb 23, 2022

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Feb 22, 2022

Removes type support from SearchHit and Explain API.

relates #1940

@nknize nknize added v2.0.0 Version 2.0.0 non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues Indexing & Search labels Feb 22, 2022
@nknize nknize requested a review from a team as a code owner February 22, 2022 00:26
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure e9546057577b15fba054473c60440e17fca1c475
Log 2681

Reports 2681

@nknize nknize force-pushed the remove/typesFromSearchHitAndExplain branch from 3365313 to b33f8e5 Compare February 22, 2022 00:55
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 3365313956f48257a602f0ae5b546db9624c8a40
Log 2683

Reports 2683

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure b33f8e5e2d1bc11d3ee806dc81d18b248e63716f
Log 2685

Reports 2685

@dreamer-89
Copy link
Member

start gradle check

dreamer-89
dreamer-89 reviewed Feb 22, 2022
View reviewed changes
modules/reindex/src/test/java/org/opensearch/index/reindex/CancelTests.java
@@ -116,7 +116,7 @@ private void testCancel(
false,
true,
IntStream.range(0, numDocs)
.mapToObj(i -> client().prepareIndex(INDEX, TYPE, String.valueOf(i)).setSource("n", i))
.mapToObj(i -> client().prepareIndex(INDEX, MapperService.SINGLE_MAPPING_NAME, String.valueOf(i)).setSource("n", i))
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove the type completely here as we are removing _doc type info from validation ymls

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Type is still needed here for cancellation tests because it hasn't yet been fully removed from the DocWriter. That'll be in a coming PR

dreamer-89
dreamer-89 reviewed Feb 22, 2022
View reviewed changes
server/src/main/java/org/opensearch/rest/action/search/RestExplainAction.java Outdated
@@ -55,20 +54,11 @@
* Rest action for computing a score explanation for specific documents.
*/
public class RestExplainAction extends BaseRestHandler {
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(RestExplainAction.class);
public static final String TYPES_DEPRECATION_MESSAGE = "[types removal] " + "Specifying a type in explain requests is deprecated.";
Copy link
Member

Choose a reason for hiding this comment

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

minor: Can we remove TYPES_DEPRECATION_MESSAGE as well ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's still needed in FullClusterRestartIT for bwc testing; so I removed it from the RestExplainAction class as a local to FullClusterRestartIT

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure b33f8e5e2d1bc11d3ee806dc81d18b248e63716f
Log 2700

Reports 2700

@nknize nknize force-pushed the remove/typesFromSearchHitAndExplain branch from b33f8e5 to 9986327 Compare February 22, 2022 19:42
@dreamer-89 dreamer-89 mentioned this pull request Feb 22, 2022
Closed
67 tasks
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9986327761b82f34424937b89b58930ecd41b4db
Log 2710

Reports 2710

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 8a45d99b1aaa418c498f1ef931c9d52c4db40189
Log 2712

Reports 2712

@nknize nknize force-pushed the remove/typesFromSearchHitAndExplain branch from 8a45d99 to 2da7968 Compare February 22, 2022 22:28
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 2da796881568a43145ac453a98e526a0b9dd6dcc
Log 2720

Reports 2720

@nknize nknize force-pushed the remove/typesFromSearchHitAndExplain branch from 2da7968 to 2d7e58d Compare February 23, 2022 16:06
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 2d7e58d4dc77d992427037e58206a47dcb1a097c
Log 2745

Reports 2745

@dreamer-89
Copy link
Member

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 2d7e58d4dc77d992427037e58206a47dcb1a097c
Log 2748

Reports 2748

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 73b1598d7c60fdc181ec59d10fc015d69a8d3073
Log 2750

Reports 2750

dreamer-89
dreamer-89 approved these changes Feb 23, 2022
View reviewed changes
Copy link
Member

@dreamer-89 dreamer-89 left a comment

Choose a reason for hiding this comment

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

LGTM

@dreamer-89
Copy link
Member

@nknize : Do you think _with_types.yml tests help in testing upgrade funcationality ?

@nknize
Copy link
Collaborator Author

nknize commented Feb 23, 2022

@nknize : Do you think _with_types.yml tests help in testing upgrade funcationality ?

no.. _with_types.yml tests have to be removed in these PRs since the type functionality and endpoints are being removed. 2.0 nodes will be unaware of the type endpoints; that's why some of the PRs fail if a _with_types.yml is missed.

@nknize
[Remove] types from SearchHit and Explain API
96b0fd4 
Removes type support from SearchHit and Explain API.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
fix failing tests
0b91b45 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
move TYPES_DEPRECATION_MESSAGE from RestExplainAction to FullClusterR…
3288195 
…estartIT

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
fix FullClusterRestartIT
7adad42 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
fix failing tests
cf63070 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize
fix testSoftDelete
b20b9a2 
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@nknize nknize force-pushed the remove/typesFromSearchHitAndExplain branch from 73b1598 to b20b9a2 Compare February 23, 2022 18:53
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success b20b9a2
Log 2753

Reports 2753

@dreamer-89 dreamer-89 merged commit 3445bef into opensearch-project:main Feb 23, 2022
@saratvemulapalli saratvemulapalli mentioned this pull request Mar 15, 2022
Closed
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamer-89 dreamer-89 dreamer-89 approved these changes

@CEHENKLE CEHENKLE CEHENKLE approved these changes

@reta reta reta approved these changes

Assignees
No one assigned
Labels
Indexing & Search non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues v2.0.0 Version 2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nknize @opensearch-ci-bot @dreamer-89 @reta @CEHENKLE