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.

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

[Type removal] Ignore _type field in bulk request #3504

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

dreamer-89
Copy link
Member

@dreamer-89 dreamer-89 commented Jun 3, 2022

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

Description

Don't fail bulk request on _type field in bulk request. This change reduces the friction with external clients which still uses _type field in bulk requests. The change include creating bulk request parser with errorOnType set to false to accommodate _type params. It also add some of test cases previously removed in #2215 to verify the assumption

Related
#2215

Issues Resolved

#3484

Testing

Request

curl localhost:9200/_bulk -H 'Content-type:application/json' -d '
{ "index" : { "_index" : "test-index", "_type":"test_type", "_id" : "1" } }
{ "field1" : "value1" }
'

Before change

{
    "error": {
        "root_cause": [
            {
                "type": "illegal_argument_exception",
                "reason": "Action/metadata line [1] contains an unknown parameter [_type]"
            }
        ],
        "type": "illegal_argument_exception",
        "reason": "Action/metadata line [1] contains an unknown parameter [_type]"
    },
    "status": 400
}

After fix

{
    "took": 209,
    "errors": false,
    "items": [
        {
            "index": {
                "_index": "test-index",
                "_id": "1",
                "_version": 2,
                "result": "updated",
                "_shards": {
                    "total": 2,
                    "successful": 1,
                    "failed": 0
                },
                "_seq_no": 2,
                "_primary_term": 1,
                "status": 200
            }
        }
    ]
}

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.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success b487511
Log 5764

Reports 5764

@dreamer-89
Copy link
Member Author

dreamer-89 commented Jun 4, 2022

Gradle check has test failures internally but reported succeeded in the end. Opened #3506

All failures belongs to 11_with_deprecated_types test file added back with type info.

  1. List of strings
REPRODUCE WITH: ./gradlew ':qa:mixed-cluster:v1.4.0#mixedClusterTest' --tests "org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=bulk/11_with_deprecated_types/List of strings}" -Dtests.seed=67F5EB33FD1D90E4 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=fi-FI -Dtests.timezone=US/Aleutian -Druntime.java=17

org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT > test {p0=bulk/11_with_deprecated_types/List of strings} FAILED
    java.lang.AssertionError: Failure at [bulk/11_with_deprecated_types:133]: expected [2xx] status code but api [count] returned [503 Service Unavailable] [{"error":{"root_cause":[],"type":"search_phase_execution_exception","reason":"all shards failed","phase":"query","grouped":true,"failed_shards":[],"stack_trace":"Failed to execute phase [query], all shards failed\n\tat org.opensearch.action.search.AbstractSearchAsyncAction.onPhaseFailure(AbstractSearchAsyncAction.java:642)\n\tat org.opensearch.action.search.AbstractSearchAsyncAction.executeNextPhase(AbstractSearchAsyncAction.java:360)\n\tat org.opensearch.action.search.AbstractSearchAsyncAction.onPhaseDone(AbstractSearchAsyncAction.java:677)\n\tat org.opensearch.action.search.AbstractSearchAsyncAction.onShardFailure(AbstractSearchAsyncAction.java:457)\n\tat org.opensearch.action.search.AbstractSearchAsyncAction.lambda$performPhaseOnShard$0(AbstractSearchAsyncAction.java:270)\n\tat org.opensearch.action.search.AbstractSearchAsyncAction$2.doRun(AbstractSearchAsyncAction.java:338)\n\tat org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:50)\n\tat org.opensearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:57)\n\tat org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:792)\n\tat org.opensearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:50)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)\n\tat java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)\n\tat java.base/java.lang.Thread.run(Thread.java:829)\n"},"status":503}]
        at __randomizedtesting.SeedInfo.seed([67F5EB33FD1D90E4:EFA1D4E953E1FD1C]:0)
  1. Array of objects
REPRODUCE WITH: ./gradlew ':qa:mixed-cluster:v1.4.0#mixedClusterTest' --tests "org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=bulk/11_with_deprecated_types/Array of objects}" -Dtests.seed=67F5EB33FD1D90E4 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=fi-FI -Dtests.timezone=US/Aleutian -Druntime.java=17

org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT > test {p0=bulk/11_with_deprecated_types/Array of objects} FAILED
    java.lang.RuntimeException: Failure at [bulk/11_with_deprecated_types:3]: 60 000 milliseconds timeout on connection http-outgoing-58 [ACTIVE]
        at __randomizedtesting.SeedInfo.seed([67F5EB33FD1D90E4:EFA1D4E953E1FD1C]:0)

  1. Empty action
1> [2022-06-03T14:32:00,739][INFO ][o.o.b.MixedClusterClientYamlTestSuiteIT] [test] [p0=bulk/11_with_deprecated_types/empty action] before test
  2> kesäkuuta 03, 2022 2:32:00 IP. org.opensearch.client.RestClient logResponse
  2> WARNING: request [POST http://[::1]:44579/_bulk?error_trace=true] returned 1 warnings: [299 OpenSearch-1.4.0-SNAPSHOT-963b1f3663df32a38ac8e3ec91dbc84ad89282fb "[types removal] Specifying types in bulk requests is deprecated."]

@andrross
Copy link
Member

andrross commented Jun 4, 2022

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success b487511
Log 5774

Reports 5774

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding the new bulk-with-deprecated-types test.

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

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

Just noticed this is going directly in the 2.0 branch. Let's first commit this to 2.x and backport to 2.0. We expect to have a better fix for future 2.x minor releases but I'm not sure that's going to come before the 2.1 release.

@dreamer-89
Copy link
Member Author

Just noticed this is going directly in the 2.0 branch. Let's first commit this to 2.x and backport to 2.0. We expect to have a better fix for future 2.x minor releases but I'm not sure that's going to come before the 2.1 release.

Thanks @nknize for the review.

Actually, I have created 2.x PR. There were conflicting changes in 2.0 and 2.x and hence created separate PRs.

@nknize
Copy link
Collaborator

nknize commented Jun 6, 2022

Actually, I have created 2.x PR. There were conflicting changes in 2.0 and 2.x and hence created separate PRs.

oic... looks like a type removal PR wasn't backported to 2.0 hence the conflicts.

@nknize nknize merged commit 5bd7236 into opensearch-project:2.0 Jun 6, 2022
dreamer-89 added a commit to dreamer-89/OpenSearch that referenced this pull request Jun 10, 2022
nknize pushed a commit that referenced this pull request Jun 10, 2022
* Revert "Revert removal of typed end-points for bulk, search, index APIs (#3524) (#3528)"

This reverts commit fc8803f.

* Revert "[Type removal] Ignore _type field in bulk request (#3504)"

This reverts commit 5bd7236.

Signed-off-by: Suraj Singh <[email protected]>
Signed-off-by: Nicholas Walter Knize <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants