Support OpenSearch 2.0 in .net client by Yury-Fridlyand · Pull Request #51 · opensearch-project/opensearch-net

Yury-Fridlyand · 2022-06-16T18:42:28Z

Description

Fixes in integration tests for OpenSearch 2.0 (OpenSearch 1.x is not affected):
- Remove check of deprecated search.remote settings.
- Remove tests for _type validation in mapping APIs as it was removed from OpenSearch.
- Remove validation for indices segments stats.
- Rename all master references to cluster_manager comply with OpenSearch 2.0
- Mark indices.exists_type/TypeExists APIs as deprecated
- Add deprecation notice to all references of include_type_name/IncludeTypeName
- Rename master_timeout to cluster_manager_timeout
Added renamed API:
- Rename master node role to cluster_manager as it was done in OpenSearch.
- CatMaster* API was duplicated into CatClusterManager*, but CatMaster* wasn't deleted. This provide backward compatibility. No new functionality added.

Issues Resolved

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.

dblock

The master/cluster manager change looks suspicious. The external API should not have changed without a major version bump and a prior deprecation, so any time we removed "master" in this code, we probably needed to mark it deprecated and call into its manager version. Then we have different enums and state values where renaming one into the other is probably similarly broken (we probably shouldn't keep the state of both master and cluster_manager, but I am not sure).

dblock · 2022-08-08T19:45:17Z

src/OpenSearch.Net.VirtualizedCluster/VirtualCluster.cs

 			return this;
 		}

-		public VirtualCluster MasterEligible(params int[] ports)


This would be a breaking change, I think you need both Master and ClusterManager versions, and tests to ensure we catch such. There's probably other similar instances.

Good point. VirtualizedCluster is used by unit tests only, but users re-use code from it or from tests in their application.
Proposed fix - recover MasterEligible and call ClusterManagerEligible inside; add deprecation notice.

@dblock,
please see fix in Bit-Quill#38.

Looking good.

Merged in 2d6cff5.

dblock

There are more instances where I think the obsolete method should be calling the non-obsolete one and be marked as such. I am not 100% sure though, I think it does work without one calling the other.

dblock · 2022-08-09T21:40:13Z

src/OpenSearch.Client/Descriptors.Cluster.cs

 		public ClusterGetSettingsDescriptor MasterTimeout(Time mastertimeout) => Qs("master_timeout", mastertimeout);
+		///<summary>Explicit operation timeout for connection to cluster_manager node</summary>
+		///<remarks>Introduced in OpenSearch 2.0 instead of <see cref="MasterTimeout"/></remarks>
+		public ClusterGetSettingsDescriptor ClusterManagerTimeout(Time timeout) => Qs("cluster_manager_timeout", timeout);


Should MasterTimeout call ClusterManagerTimeout and be deprecated with @Obsolete?

I didn't use here Obsolete attribute, because this is not obsolete for OpenSearch 1.x. This could fail build for users.

I don't call MasterTimeout in ClusterManagerTimeout, because these methods set different parameters. Imaging you are working with OpenSearch 1.2 and setting MasterTimeout. If it sets cluster_manager_timeout under the hood, server won't recognize it.
We can call MasterTimeout in ClusterManagerTimeout in addition to setting cluster_manager_timeout, but I think it is odd.

This is applicable for all instances of ClusterManagerTimeout (a lots of them).

Ok, you're absolutely right.

src/OpenSearch.Client/Descriptors.Cluster.cs

src/OpenSearch.Client/OpenSearchClient.Cat.cs

src/OpenSearch.Client/Requests.Cluster.cs

dblock

This is great! I think the one thing missing is incrementing the client version to 1.1. If I am not mistaken, semantically this version has a new feature (support for OpenSearch 2.x) and doesn't contain any breaking changes, so it shouldn't be 2.0, but 1.1.

dblock · 2022-08-16T17:09:42Z

COMPATIBILITY.md

+| 1.3.2 | 1.0.0 |
+| 1.3.3 | 1.0.0 |
+| 2.0.0 | 1.0.0 |
+| 2.0.1 | 1.0.0 |


I think we should update the compatibility matrix to say that client 1.0 is compatible with OpenSearch 1.x, and client 1.1 is compatible with OpenSearch 1.x and 2.x.

Agree, if 1.x means literally 1.x. I asked in inital PR to use 1.x/2.x instead of listing out all versions, it's easier to read.

The next client version should be 1.1 since there are no breaking changes, so maybe this? Users are likely to be already using some version of OpenSearch. They can lookup what are the compatible client versions, instead of the other way around.

| OpenSearch Version | Client Version | |--------------------|----------------| | 1.x | 1.0.0, 1.1.0 | | 2.x | 1.1.0 |

You could reverse the columns making it easier to grok ;)

Thanks, updated in 0d58b5b.

COMPATIBILITY.md

MaxKsyunz · 2022-08-29T23:33:40Z

@dblock do you mind having a look at this again? Rebasing dismissed existing reviews.

wbeckler

LGTM given there were two reviewers okay already.

Yury-Fridlyand requested review from anirudha and joshuali925 June 16, 2022 18:46

acarbonetto previously approved these changes Jul 12, 2022

View reviewed changes

Yury-Fridlyand marked this pull request as ready for review August 3, 2022 17:50

Yury-Fridlyand requested a review from a team as a code owner August 3, 2022 17:50

Yury-Fridlyand changed the title ~~Essential support OpenSearch 2.0 in .net client~~ Support OpenSearch 2.0 in .net client Aug 3, 2022

anirudha previously approved these changes Aug 5, 2022

View reviewed changes

joshuali925 previously approved these changes Aug 5, 2022

View reviewed changes

Yury-Fridlyand dismissed stale reviews from joshuali925, anirudha, and acarbonetto via a88e99b August 5, 2022 16:38

joshuali925 previously approved these changes Aug 5, 2022

View reviewed changes

acarbonetto previously approved these changes Aug 8, 2022

View reviewed changes

dblock requested changes Aug 8, 2022

View reviewed changes

This was referenced Aug 9, 2022

Recover MasterEligible Bit-Quill/opensearch-net#38

Merged

1.1 .Net client release with OpenSearch 2.0 support #76

Closed

dblock reviewed Aug 9, 2022

View reviewed changes

MaxKsyunz mentioned this pull request Aug 11, 2022

[BUG] Invalid response on Bulk actions after success #72

Closed

Yury-Fridlyand dismissed stale reviews from acarbonetto and joshuali925 via 13bf5d4 August 15, 2022 18:18

Yury-Fridlyand requested review from anirudha, dblock and joshuali925 August 15, 2022 18:25

joshuali925 previously approved these changes Aug 15, 2022

View reviewed changes

dblock reviewed Aug 16, 2022

View reviewed changes

dblock mentioned this pull request Aug 17, 2022

Document that clients are compatible across at least 2 major versions opensearch-project/opensearch-clients#17

Open

Yury-Fridlyand dismissed joshuali925’s stale review via 8ba377a August 22, 2022 23:42

Yury-Fridlyand requested review from acarbonetto and dblock and removed request for acarbonetto August 23, 2022 21:23

Yury-Fridlyand dismissed stale reviews from dblock, joshuali925, and MaxKsyunz via 234b745 August 29, 2022 22:03

Yury-Fridlyand force-pushed the dev-os-2.0-integ branch from 571669b to 234b745 Compare August 29, 2022 22:03

Yury-Fridlyand requested review from MaxKsyunz, acarbonetto, dblock and joshuali925 and removed request for MaxKsyunz and acarbonetto August 29, 2022 22:28

joshuali925 approved these changes Aug 29, 2022

View reviewed changes

MaxKsyunz approved these changes Aug 29, 2022

View reviewed changes

This was referenced Aug 30, 2022

[FEATURE] Add support for inclusive naming #89

Closed

Release .net client abstractions opensearch-project/opensearch-net-abstractions#16

Closed

PoC: using local abstractions sources instead of packages Bit-Quill/opensearch-net#40

Closed

wbeckler approved these changes Aug 31, 2022

View reviewed changes

wbeckler merged commit 835c266 into opensearch-project:main Aug 31, 2022

Yury-Fridlyand deleted the dev-os-2.0-integ branch September 9, 2022 21:13

This was referenced Sep 15, 2022

Breaking changes for 2.0 #34

Closed

Add readme for nuget packages #80

Closed

Conversation

Yury-Fridlyand commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Uh oh!

dblock left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Yury-Fridlyand Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dblock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshuali925 Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MaxKsyunz commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbeckler left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Yury-Fridlyand commented Jun 16, 2022 •

edited

Loading

dblock left a comment •

edited

Loading

Yury-Fridlyand Aug 9, 2022 •

edited

Loading

joshuali925 Aug 17, 2022 •

edited

Loading

MaxKsyunz commented Aug 29, 2022 •

edited

Loading