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

Removes types from mapping APIs #2238

Merged
merged 1 commit into from
Feb 24, 2022

Conversation

dreamer-89
Copy link
Member

Description

With types deprecation, this change removes types API end-points from GetMapping, PutMapping & GetFieldMapping actions.

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.

Fix java syntax issues

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

Signed-off-by: Suraj Singh <[email protected]>
@dreamer-89 dreamer-89 requested a review from a team as a code owner February 24, 2022 17:25
@opensearch-ci-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@dreamer-89 dreamer-89 mentioned this pull request Feb 24, 2022
67 tasks
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9e413b4
Log 2773

Reports 2773

@dreamer-89
Copy link
Member Author

start gradle check

@nknize
Copy link
Collaborator

nknize commented Feb 24, 2022

The failure looks unrelated but I think we need a deeper look into what's going on. I'm not sure this is a flake; maybe a test configuration that could use adjustment?

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testCoordinatingPrimaryThreadedUpdateToShardLimits" -Dtests.seed=D6BACFF135EA7C0B -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=bg -Dtests.timezone=Asia/Novosibirsk -Druntime.java=17
  2> java.lang.AssertionError
        at __randomizedtesting.SeedInfo.seed([D6BACFF135EA7C0B:44EAAEC902695D33]:0)
        at org.junit.Assert.fail(Assert.java:87)
        at org.junit.Assert.assertTrue(Assert.java:42)
        at org.junit.Assert.assertTrue(Assert.java:53)
        at org.opensearch.index.ShardIndexingPressureConcurrentExecutionTests.testCoordinatingPrimaryThreadedUpdateToShardLimits(ShardIndexingPressureConcurrentExecutionTests.java:74)

At minimum we should make the assertions more informative, e.g.:

        assertTrue( "Primary and Coordinating Shard Pressure limits exceed threshold of 95%", 
            (double) (NUM_THREADS * 15) / shardIndexingPressure.shardStats()
                .getIndexingPressureShardStats(shardId1)
                .getCurrentPrimaryAndCoordinatingLimits() > 0.75
        );

/cc @getsaurabh02

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.

Nice.. this LGTM

@@ -68,15 +68,7 @@

@Override
public List<Route> routes() {
return unmodifiableList(
asList(
new Route(GET, "/_mapping/field/{fields}"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Let's try to keep these in a column for readability?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like java spotless check complains when I keep remaining two end-points on separate rows.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9e413b4
Log 2777

Reports 2777

@dreamer-89
Copy link
Member Author

Another gradle check failure. I tried to run this test in a loop but not able to repro on my mac

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.search.SearchCancellationIT.testMSearchChildRequestCancellationWithClusterLevelTimeout" -Dtests.seed=36921F5835FAE3A1 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-IN -Dtests.timezone=Etc/GMT+5 -Druntime.java=17

org.opensearch.search.SearchCancellationIT > testMSearchChildRequestCancellationWithClusterLevelTimeout FAILED
    java.lang.AssertionError: Actual child request with cancellation failure is different that expected expected:<[0, 1]> but was:<[]>
        at __randomizedtesting.SeedInfo.seed([36921F5835FAE3A1:882FD4BDA4EC03D6]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.junit.Assert.failNotEquals(Assert.java:835)
        at org.junit.Assert.assertEquals(Assert.java:120)
        at org.opensearch.search.SearchCancellationIT.ensureMSearchWasCancelled(SearchCancellationIT.java:194)
        at org.opensearch.search.SearchCancellationIT.testMSearchChildRequestCancellationWithClusterLevelTimeout(SearchCancellationIT.java:537)

@dreamer-89
Copy link
Member Author

start gradle check

@dreamer-89
Copy link
Member Author

I will run gradle check in a loop to see if I can reproduce any of above failures more consistently in ubuntu machine.

}
},
{
"path":"/{index}/_mappings",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why this test case have been removed? It is legitimate /{index}/_mappings, 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.

Hi @reta ,
Thanks, for pointing out. You are right, this is a legitimate api spec. Let me add this back in a follow up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @reta,

Sorry for delay on my end to this. I just noticed that _mappings specific spec is marked deprecated as pasted below. Looks like _mappings is only accepted but not documented. Please let me know your thoughts on this.

"deprecated":{
            "version":"7.0.0",
            "description":"The plural mappings is accepted but only /_mapping is documented"
          }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah .. I see, missed that, thanks @dreamer-89 !

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9e413b4
Log 2781

Reports 2781

@dreamer-89
Copy link
Member Author

I will merge these changes and open new issues to track these transient failures.
I haven't been able to repro them in last 5 gradle check runs on ubuntu machine. I will probably run checks overnight to see if we are seeing any of above two failures (to rule out any regression from recent changes).

@dreamer-89 dreamer-89 merged commit cbfcad9 into opensearch-project:main Feb 24, 2022
@dreamer-89 dreamer-89 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 >breaking Identifies a breaking change. Indexing & Search labels Mar 21, 2022
Yury-Fridlyand added a commit to Bit-Quill/opensearch-net that referenced this pull request Jun 9, 2022
Yury-Fridlyand added a commit to Bit-Quill/opensearch-net that referenced this pull request Aug 29, 2022
* Update nuget packages (opensearch-project#88)
* Cleanup - removing stale dependencies.
* Fix 2 typos in scripts project
* Update `scripts` project file to include all relevant objects.
* Rename `master` node role to `cluster_manager` as it was done in OpenSearch.
    Ref: opensearch-project/OpenSearch#2480
* Remove validation for indices segments stats.
    `OpenSearch` 2.0 uses newer version of `Lucene` (9.0) which doesn't provide segments stats info.
    Ref: opensearch-project/OpenSearch#2029 opensearch-project/OpenSearch#1109
    See also history for `server/src/main/java/org/opensearch/index/engine/SegmentsStats.java` in `OpenSearch` repo.
* Remove tests for `_type` validation in mapping APIs as it was removed from `OpenSearch`.
    Ref: opensearch-project/OpenSearch#2238 opensearch-project/OpenSearch#2480
* Remove usage of deprecated `search.remote` settings.
    Ref: opensearch-project/OpenSearch#1870
* Update abstractions package - patch to support OpenSearch 2.0. Update integration workflow to run tests on OpenSearch 2.0.
* Rename `master_timeout` to `cluster_manager_timeout` in all APIs where it is used.
* Enrich comments to already renamed `CatMaster`/`CatClusterManager` API.
* Rename in `/_cluster/stats`/`cluster.stats` and `/_cluster/state`/`cluster.state`.
* Add deprecation info.
* Rename in comments.
* Rename in test data.
* Renamings in tests including `MasterEligible`, but mark it is obsolete.
* Rename branch reference in scripting.
* Mark `indices.exists_type`/`TypeExists` APIs as deprecated.
* Update compatibility matrix and include it into `sln` file.
* Add deprecation notice to all reference of `include_type_name`/`IncludeTypeName`.
* Update compatibility matrix.
* Remove `OpenDistro` compatibility notice.
* Update repo link.
* Add small README for each project being released.
* Address PR opensearch-project#51 feedback.

Signed-off-by: Yury-Fridlyand <[email protected]>
wbeckler pushed a commit to opensearch-project/opensearch-net that referenced this pull request Aug 31, 2022
* Update nuget packages (#88)
* Cleanup - removing stale dependencies.
* Fix 2 typos in scripts project
* Update `scripts` project file to include all relevant objects.
* Rename `master` node role to `cluster_manager` as it was done in OpenSearch.
    Ref: opensearch-project/OpenSearch#2480
* Remove validation for indices segments stats.
    `OpenSearch` 2.0 uses newer version of `Lucene` (9.0) which doesn't provide segments stats info.
    Ref: opensearch-project/OpenSearch#2029 opensearch-project/OpenSearch#1109
    See also history for `server/src/main/java/org/opensearch/index/engine/SegmentsStats.java` in `OpenSearch` repo.
* Remove tests for `_type` validation in mapping APIs as it was removed from `OpenSearch`.
    Ref: opensearch-project/OpenSearch#2238 opensearch-project/OpenSearch#2480
* Remove usage of deprecated `search.remote` settings.
    Ref: opensearch-project/OpenSearch#1870
* Update abstractions package - patch to support OpenSearch 2.0. Update integration workflow to run tests on OpenSearch 2.0.
* Rename `master_timeout` to `cluster_manager_timeout` in all APIs where it is used.
* Enrich comments to already renamed `CatMaster`/`CatClusterManager` API.
* Rename in `/_cluster/stats`/`cluster.stats` and `/_cluster/state`/`cluster.state`.
* Add deprecation info.
* Rename in comments.
* Rename in test data.
* Renamings in tests including `MasterEligible`, but mark it is obsolete.
* Rename branch reference in scripting.
* Mark `indices.exists_type`/`TypeExists` APIs as deprecated.
* Update compatibility matrix and include it into `sln` file.
* Add deprecation notice to all reference of `include_type_name`/`IncludeTypeName`.
* Update compatibility matrix.
* Remove `OpenDistro` compatibility notice.
* Update repo link.
* Add small README for each project being released.
* Address PR #51 feedback.

Signed-off-by: Yury-Fridlyand <[email protected]>

Signed-off-by: Yury-Fridlyand <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking Identifies a breaking change. 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants