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] Type mapping parameter from document delete API #2213

Closed
wants to merge 4 commits into from
Closed

[Remove] Type mapping parameter from document delete API #2213

wants to merge 4 commits into from

Conversation

dreamer-89
Copy link
Member

Description

With types deprecation, this change removes type mapping API end-points provided for document update API end-point.

Relates #1940

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.

@dreamer-89
Remove type param from yml and integ test files
74e1c2e 
Signed-off-by: Suraj Singh <surajrider@gmail.com>
@dreamer-89
Remove type mapping specific update API
6197384 
Signed-off-by: Suraj Singh <surajrider@gmail.com>
@dreamer-89
Remove type mapping specific delete API
449b5ec 
Signed-off-by: Suraj Singh <surajrider@gmail.com>
@dreamer-89 dreamer-89 requested a review from a team as a code owner February 22, 2022 18:29
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@dreamer-89 dreamer-89 mentioned this pull request Feb 22, 2022
Closed
67 tasks
@dreamer-89
Copy link
Member Author

This needs to be rebased against main once #2204 is merged.

reta
reta reviewed Feb 22, 2022
View reviewed changes
server/src/main/java/org/opensearch/rest/action/document/RestDeleteAction.java Outdated
new Route(DELETE, "/{index}/{type}/{id}")
)
);
return unmodifiableList(asList(new Route(DELETE, "/{index}/_doc/{id}")));
Copy link
Collaborator

@reta reta Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't it be just "/{index}/{id}"? Afaik, the mapping might be changed from _doc, but the key concept - only single mapping allowed - will stay. May be even better to support both:

asList(
                new Route(DELETE, "/{index}/_doc/{id}"),
                new Route(DELETE, "/{index}/{id}")
            )

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for taking time and reviewing this PR.

Yes, keeping both suggested end-points make sense.
The only point I have it against is document index APIs have either _doc or _create in the path where it provides document index behaviour instead of document type.

But, I think for delete action, we don't need this segregation and we can use both end-points as you suggested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 makes sense, thanks!

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 449b5ec
Log 2705

Reports 2705

reta
reta reviewed Feb 22, 2022
View reviewed changes
server/src/main/java/org/opensearch/rest/action/document/RestDeleteAction.java Outdated

@Override
public List<Route> routes() {
return unmodifiableList(
asList(
new Route(DELETE, "/{index}/_doc/{id}"),
// Deprecated typed endpoint.
new Route(DELETE, "/{index}/{type}/{id}")
new Route(DELETE, "/{index}//{id}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, I think we need only one slash, right? // -> /

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@dreamer-89 dreamer-89 force-pushed the type_remove_rest_delete branch from 32c9579 to e61001c Compare February 22, 2022 19:27
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 32c9579e3d0fcb551335b78d81a20064bb37adf5
Log 2707

Reports 2707

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure e61001cc6de412cdeb156fd739d37ce9d8331a0e
Log 2708

Reports 2708

@dreamer-89
Address review comments
af79400 
Signed-off-by: Suraj Singh <surajrider@gmail.com>
@dreamer-89 dreamer-89 force-pushed the type_remove_rest_delete branch from e61001c to af79400 Compare February 22, 2022 19:49
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure af79400
Log 2711

Reports 2711

@dreamer-89
Copy link
Member Author

Update: Looks like adding DELETE {index}/{id} end-point back introduces bunch of test failures. One sample failure below. Checking further.

REPRODUCE WITH: ./gradlew ':modules:parent-join:test' --tests "org.opensearch.join.mapper.ParentJoinFieldMapperTests.testEagerGlobalOrdinals" -Dtests.seed=92A22F9746A35A40 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=zh-HK -Dtests.timezone=Asia/Bangkok -Druntime.java=17

org.opensearch.join.mapper.ParentJoinFieldMapperTests > testEagerGlobalOrdinals FAILED
    java.lang.IllegalArgumentException: Trying to use conflicting wildcard names for same path: type and id
        at org.opensearch.common.path.PathTrie$TrieNode.updateKeyWithNamedWildcard(PathTrie.java:116)
        at org.opensearch.common.path.PathTrie$TrieNode.insertOrUpdate(PathTrie.java:179)
        at org.opensearch.common.path.PathTrie$TrieNode.insertOrUpdate(PathTrie.java:194)
        at org.opensearch.common.path.PathTrie$TrieNode.access$400(PathTrie.java:91)
        at org.opensearch.common.path.PathTrie.insertOrUpdate(PathTrie.java:339)

@reta
Copy link
Collaborator

reta commented Feb 22, 2022

Update: Looks like adding DELETE {index}/{id} end-point back introduces bunch of test failures. One sample failure below. Checking further.

REPRODUCE WITH: ./gradlew ':modules:parent-join:test' --tests "org.opensearch.join.mapper.ParentJoinFieldMapperTests.testEagerGlobalOrdinals" -Dtests.seed=92A22F9746A35A40 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=zh-HK -Dtests.timezone=Asia/Bangkok -Druntime.java=17

org.opensearch.join.mapper.ParentJoinFieldMapperTests > testEagerGlobalOrdinals FAILED
    java.lang.IllegalArgumentException: Trying to use conflicting wildcard names for same path: type and id
        at org.opensearch.common.path.PathTrie$TrieNode.updateKeyWithNamedWildcard(PathTrie.java:116)
        at org.opensearch.common.path.PathTrie$TrieNode.insertOrUpdate(PathTrie.java:179)
        at org.opensearch.common.path.PathTrie$TrieNode.insertOrUpdate(PathTrie.java:194)
        at org.opensearch.common.path.PathTrie$TrieNode.access$400(PathTrie.java:91)
        at org.opensearch.common.path.PathTrie.insertOrUpdate(PathTrie.java:339)

Interesting, probably the type should be gone completely ... @nknize wdyt?

@dreamer-89
Copy link
Member Author

Resolving in favour of #2239

@dreamer-89 dreamer-89 closed this Feb 25, 2022
@dreamer-89 dreamer-89 deleted the type_remove_rest_delete branch December 14, 2022 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CEHENKLE CEHENKLE CEHENKLE approved these changes

@reta reta reta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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