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

Breaking changes for type removal #132

Merged

Conversation

VachaShah
Copy link
Collaborator

@VachaShah VachaShah commented Mar 30, 2022

Description

This PR contains the first set of breaking changes for 2.0 related to OpenSearch 2.0 - opensearch-project/OpenSearch#2480

The following changes are included in this PR:

  1. A CI workflow test-integration-unreleased.yml which runs integ tests for the client against unreleased OpenSearch. This will help in future development to prepare clients with the latest unreleased OpenSearch versions. Currently this workflow will test against 2.0 OpenSearch.
  2. The CI workflow test-integration.yml is updated to work for released versions which the client is compatible with.
  3. Upgraded the OpenSearch version in the client to 2.0.0-alpha1-SNAPSHOT.
  4. Type mapping removal across the APIs and related test changes.
  5. Remove indices.exists_type and indices.flush_synced and related files.

Issues Resolved

Part of #139

Check List

  • Commits are signed per the DCO using --signoff

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.

@VachaShah VachaShah requested review from a team and madhusudhankonda as code owners March 30, 2022 18:32
@@ -17,6 +17,19 @@ jobs:
with:
java-version: '14'
distribution: 'adopt'

- name: Checkout OpenSearch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm ... why we are building opensearch-project/opensearch instead of using published snapshots from https://aws.oss.sonatype.org/content/repositories/snapshots/?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah my bad! I can use that for build and unit test. I will update the workflows for the same.

if (propsSet == (_index | _type)) {
StringBuilder buf = new StringBuilder();
buf.append("/");
SimpleEndpoint.pathEncode(request.index, buf);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like _index should not be dropped

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah ... not visible in changeset, thank you!

# Reference: https://github.com/opensearch-project/OpenSearch/blob/2.0/distribution/docker/build.gradle#L190
- name: Run Docker Image
run: |
docker run -p 9200:9200 -p 9600:9600 -d -e "discovery.type=single-node" -e "bootstrap.memory_lock=true" opensearch:test
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am 100% sure there is a staging repository for Docker images, @dblock @peterzhuamazon could you please refer to the right one?

Copy link
Collaborator Author

@VachaShah VachaShah Mar 30, 2022

Choose a reason for hiding this comment

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

Yes there is one, but the staging image is with all the plugins which isn't ready yet. This is to test against min OpenSearch image. This also helps in client development when a new version of OpenSearch is being developed.

Copy link
Collaborator Author

@VachaShah VachaShah Mar 30, 2022

Choose a reason for hiding this comment

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

I also tried the route of writing Dockerfile and related things to create a docker image for min OpenSearch and use the script in opensearch-build to use the build and run the image. But then I found this gradle task that runs in assemble for OpenSearch so used this one. LMK if you think the Dockerfile route is better. I can use that instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think publishing min Docker images (along with artifacts) would be the best option, may be we could quickly address that? Another possibility is to use OpenSearch test scaffolding (available as set of Gradle plugins) to run clusters from the test suites. What do you think?

Copy link
Collaborator Author

@VachaShah VachaShah Mar 30, 2022

Choose a reason for hiding this comment

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

I didn't use the Gradle plugins since I wanted to create a CI way for all the clients. It can work for java client but we would still end up in the same place for other clients. The main purpose of this is for clients to be ready to be released when a new version of OpenSearch is being released.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, thank you, publishing min Docker images (along with artifacts) would be the answer, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that would be the best option. Till that happens, is it fine if we use this change so we can start the development for 2.0? WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, sure, I think it is viable option

Copy link
Member

@VijayanB VijayanB Mar 31, 2022

Choose a reason for hiding this comment

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

I believe we will be adding API for plugins too and we need unreleased complete distribution for testing. Shall we create an issue to track this if it is not created already?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, please do, I think it will useful for many

@VachaShah
Copy link
Collaborator Author

I will merge the changes from #134 once that is approved and merged and update the workflows with the appropriate JDK versions.

reta
reta previously approved these changes Mar 30, 2022
CEHENKLE
CEHENKLE previously approved these changes Mar 30, 2022
@VachaShah VachaShah dismissed stale reviews from CEHENKLE and reta via c8b8720 April 1, 2022 01:09
@VachaShah VachaShah force-pushed the breaking-changes-remove-type branch from 35b7c06 to c8b8720 Compare April 1, 2022 01:09
@VachaShah VachaShah force-pushed the breaking-changes-remove-type branch from c8b8720 to d82f4b6 Compare April 1, 2022 03:34
Signed-off-by: Vacha Shah <[email protected]>
@VachaShah
Copy link
Collaborator Author

@reta @CEHENKLE @VijayanB A review again please? Just merged the changes from the main branch for the workflows and removed indices.exists_type as part of the breaking changes.

@owaiskazi19
Copy link
Member

@VachaShah I see java client is using checkstyle for formatting. Moving forward do we have any plan to have Spotless as the formatting tool?

@VachaShah
Copy link
Collaborator Author

@VachaShah I see java client is using checkstyle for formatting. Moving forward do we have any plan to have Spotless as the formatting tool?

No plans as such but you can create a PR :)

Copy link
Member

@VijayanB VijayanB 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 making the changes.

@VachaShah
Copy link
Collaborator Author

Not sure why the whitesource check is failing, the dependency was updated in #129. Will keep an eye on it.

val jacksonVersion = "2.12.6"
val jacksonDatabindVersion = "2.12.6.1"

// Apache 2.0
implementation("org.opensearch.client", "opensearch-rest-client", opensearchVersion)
testImplementation("org.opensearch.test", "framework", opensearchVersion)
implementation("org.apache.logging.log4j", "log4j-core", "2.17.2")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this introducing a hard-coded dependency on the Log4j framework? Surely the client should only be dependent on the facade?

Copy link
Member

Choose a reason for hiding this comment

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

I think you may be right @dermot-hardy! Care to open an issue/contribute a fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants