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

Breaking changes for type removal #132

Merged
merged 6 commits into from
Apr 1, 2022
Merged

Breaking changes for type removal #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
adac661
Adding support to test against unreleased OpenSearch
VachaShah Mar 30, 2022
e629651
Upgrading OpenSearch version to 2.0.0-SNAPSHOT and Java version
VachaShah Mar 30, 2022
5898cc0
Remove type mapping
VachaShah Mar 30, 2022
a8d1300
Using snapshots from sonatype for 2.0 OpenSearch snapshot
VachaShah Mar 30, 2022
d82f4b6
Updating workflows and removing exists_type
VachaShah Apr 1, 2022
0b0a118
Fixing integration tests
VachaShah Apr 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions .github/workflows/test-integration-unreleased.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
name: Integration with Unreleased OpenSearch

on:
push:
branches:
- "main"
pull_request:
branches:
- "main"

env:
OPENSEARCH_VERSION: '2.0'

jobs:
test:
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
java: [ 11 ]
steps:
- name: Set up JDK ${{ matrix.java }}
uses: actions/setup-java@v1
with:
java-version: ${{ matrix.java }}

- name: Checkout OpenSearch
uses: actions/checkout@v2
with:
repository: opensearch-project/opensearch
ref: ${{ env.OPENSEARCH_VERSION }}
path: opensearch

- name: Assemble OpenSearch
run: |
cd opensearch
./gradlew assemble

# This step runs the docker image generated during gradle assemble in OpenSearch. It is tagged as opensearch:test.
# 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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

sleep 90

- name: Checkout Java Client
uses: actions/checkout@v2
with:
path: opensearch-java

- name: Run Integration Test
run: |
cd opensearch-java
./gradlew clean integrationTest -Dhttps=false
22 changes: 9 additions & 13 deletions .github/workflows/test-integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
name: Integration
name: Integration with Released OpenSearch

on:
push:
Expand All @@ -15,33 +15,29 @@ jobs:
fail-fast: false
matrix:
entry:
- { opensearch_version: 1.0.0, java: 11 }
- { opensearch_version: 1.0.1, java: 11 }
- { opensearch_version: 1.1.0, java: 11 }
- { opensearch_version: 1.2.0, java: 11 }
- { opensearch_version: 1.2.1, java: 11 }
- { opensearch_version: 1.2.2, java: 11 }
- { opensearch_version: 1.2.3, java: 11 }
- { opensearch_version: 1.2.4, java: 11 }
- { opensearch_version: 1.3.0, java: 11 }
- { opensearch_version: "", java: 11 }
steps:
- name: Set up JDK ${{ matrix.entry.java }}
- name: Set up JDK ${{ matrix.java }}
if: ${{ matrix.entry.opensearch_version != ''}}
uses: actions/setup-java@v1
with:
java-version: ${{ matrix.entry.java }}

- name: Checkout Branch
if: ${{ matrix.entry.opensearch_version != ''}}
uses: actions/checkout@v2

- name: Run Docker
if: ${{ matrix.entry.opensearch_version != ''}}
run: |
docker-compose --project-directory .ci/opensearch build --build-arg OPENSEARCH_VERSION=${{ matrix.entry.opensearch_version }}
docker-compose --project-directory .ci/opensearch up -d
sleep 60

- name: Run Integration Test
if: ${{ matrix.entry.opensearch_version != ''}}
run: ./gradlew clean integrationTest

- name: Stop Docker
if: ${{ matrix.entry.opensearch_version != ''}}
run: |
docker-compose --project-directory .ci/opensearch down
docker-compose --project-directory .ci/opensearch down
3 changes: 2 additions & 1 deletion build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ allprojects {
(File(project.rootDir, "config/version.txt").readText().trim() + "-SNAPSHOT")

repositories {
mavenCentral()
mavenLocal()
maven(url = "https://aws.oss.sonatype.org/content/repositories/snapshots")
mavenCentral()
maven(url = "https://plugins.gradle.org/m2/")
}

Expand Down
3 changes: 2 additions & 1 deletion buildSrc/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ dependencies {
}

repositories {
mavenCentral()
mavenLocal()
maven(url = "https://aws.oss.sonatype.org/content/repositories/snapshots")
mavenCentral()
maven(url = "https://plugins.gradle.org/m2/")
}
10 changes: 6 additions & 4 deletions java-client/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ import java.io.FileWriter

buildscript {
repositories {
mavenCentral()
mavenLocal()
maven(url = "https://aws.oss.sonatype.org/content/repositories/snapshots")
mavenCentral()
maven(url = "https://plugins.gradle.org/m2/")
}
}
Expand All @@ -55,8 +56,8 @@ checkstyle {
}

java {
targetCompatibility = JavaVersion.VERSION_1_8
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_11
sourceCompatibility = JavaVersion.VERSION_11

withJavadocJar()
withSourcesJar()
Expand Down Expand Up @@ -122,13 +123,14 @@ val integrationTest = task<Test>("integrationTest") {

dependencies {

val opensearchVersion = "1.2.4"
val opensearchVersion = "2.0.0-alpha1-SNAPSHOT"
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.

#175


// Apache 2.0
// https://search.maven.org/artifact/com.google.code.findbugs/jsr305
Expand Down
29 changes: 0 additions & 29 deletions java-client/src/main/java/org/opensearch/client/opensearch/_types/WriteResponseBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,6 @@ public abstract class WriteResponseBase implements JsonpSerializable {

private final ShardStatistics shards;

@Nullable
private final String type;

private final long version;

@Nullable
Expand All @@ -81,7 +78,6 @@ protected WriteResponseBase(AbstractBuilder<?> builder) {
this.result = ApiTypeHelper.requireNonNull(builder.result, this, "result");
this.seqNo = ApiTypeHelper.requireNonNull(builder.seqNo, this, "seqNo");
this.shards = ApiTypeHelper.requireNonNull(builder.shards, this, "shards");
this.type = builder.type;
this.version = ApiTypeHelper.requireNonNull(builder.version, this, "version");
this.forcedRefresh = builder.forcedRefresh;

Expand Down Expand Up @@ -129,14 +125,6 @@ public final ShardStatistics shards() {
return this.shards;
}

/**
* API name: {@code _type}
*/
@Nullable
public final String type() {
return this.type;
}

/**
* Required - API name: {@code _version}
*/
Expand Down Expand Up @@ -180,11 +168,6 @@ protected void serializeInternal(JsonGenerator generator, JsonpMapper mapper) {
generator.writeKey("_shards");
this.shards.serialize(generator, mapper);

if (this.type != null) {
generator.writeKey("_type");
generator.write(this.type);

}
generator.writeKey("_version");
generator.write(this.version);

Expand All @@ -211,9 +194,6 @@ protected abstract static class AbstractBuilder<BuilderT extends AbstractBuilder

private ShardStatistics shards;

@Nullable
private String type;

private Long version;

@Nullable
Expand Down Expand Up @@ -274,14 +254,6 @@ public final BuilderT shards(Function<ShardStatistics.Builder, ObjectBuilder<Sha
return this.shards(fn.apply(new ShardStatistics.Builder()).build());
}

/**
* API name: {@code _type}
*/
public final BuilderT type(@Nullable String value) {
this.type = value;
return self();
}

/**
* Required - API name: {@code _version}
*/
Expand Down Expand Up @@ -312,7 +284,6 @@ protected static <BuilderT extends AbstractBuilder<BuilderT>> void setupWriteRes
op.add(AbstractBuilder::result, Result._DESERIALIZER, "result");
op.add(AbstractBuilder::seqNo, JsonpDeserializer.longDeserializer(), "_seq_no");
op.add(AbstractBuilder::shards, ShardStatistics._DESERIALIZER, "_shards");
op.add(AbstractBuilder::type, JsonpDeserializer.stringDeserializer(), "_type");
op.add(AbstractBuilder::version, JsonpDeserializer.longDeserializer(), "_version");
op.add(AbstractBuilder::forcedRefresh, JsonpDeserializer.booleanDeserializer(), "forced_refresh");

Expand Down
39 changes: 0 additions & 39 deletions java-client/src/main/java/org/opensearch/client/opensearch/core/BulkRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,6 @@ public class BulkRequest extends RequestBase implements NdJsonpSerializable, Jso
@Nullable
private final Time timeout;

@Nullable
private final String type;

@Nullable
private final WaitForActiveShards waitForActiveShards;

Expand All @@ -116,7 +113,6 @@ private BulkRequest(Builder builder) {
this.requireAlias = builder.requireAlias;
this.routing = builder.routing;
this.timeout = builder.timeout;
this.type = builder.type;
this.waitForActiveShards = builder.waitForActiveShards;
this.operations = ApiTypeHelper.unmodifiableRequired(builder.operations, this, "operations");

Expand Down Expand Up @@ -224,16 +220,6 @@ public final Time timeout() {
return this.timeout;
}

/**
* Default document type for items which don't provide one
* <p>
* API name: {@code type}
*/
@Nullable
public final String type() {
return this.type;
}

/**
* Sets the number of shard copies that must be active before proceeding with
* the bulk operation. Defaults to 1, meaning the primary shard only. Set to
Expand Down Expand Up @@ -304,9 +290,6 @@ public static class Builder extends ObjectBuilderBase implements ObjectBuilder<B
@Nullable
private Time timeout;

@Nullable
private String type;

@Nullable
private WaitForActiveShards waitForActiveShards;

Expand Down Expand Up @@ -457,16 +440,6 @@ public final Builder timeout(Function<Time.Builder, ObjectBuilder<Time>> fn) {
return this.timeout(fn.apply(new Time.Builder()).build());
}

/**
* Default document type for items which don't provide one
* <p>
* API name: {@code type}
*/
public final Builder type(@Nullable String value) {
this.type = value;
return this;
}

/**
* Sets the number of shard copies that must be active before proceeding with
* the bulk operation. Defaults to 1, meaning the primary shard only. Set to
Expand Down Expand Up @@ -559,14 +532,11 @@ public BulkRequest build() {
// Request path
request -> {
final int _index = 1 << 0;
final int _type = 1 << 1;

int propsSet = 0;

if (request.index() != null)
propsSet |= _index;
if (request.type() != null)
propsSet |= _type;

if (propsSet == 0) {
StringBuilder buf = new StringBuilder();
Expand All @@ -580,15 +550,6 @@ public BulkRequest build() {
buf.append("/_bulk");
return buf.toString();
}
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.

The _index part is before this if condition. https://github.com/opensearch-project/opensearch-java/pull/132/files/1e01fbeae03463bf2d47b8d395bce3701901448d#diff-2bd95eccdb4274c5118be928fb9f466034f19fc2cdcfc3e5c82609585af94e0aL576

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!

buf.append("/");
SimpleEndpoint.pathEncode(request.type, buf);
buf.append("/_bulk");
return buf.toString();
}
throw SimpleEndpoint.noPathTemplateFound("path");

},
Expand Down
Loading