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

[Plugin migration] Update upstream #1

Merged
merged 2 commits into from
Apr 12, 2021

Conversation

VijayanB
Copy link
Member

Updated build file to use openserach-7.10.3-SNAPSHOT as upstream.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Updated build file to use openserach-7.10.3-SNAPSHOT as upstream.
@VijayanB VijayanB requested review from jmazanec15 and vamshin and removed request for jmazanec15 April 10, 2021 20:40
@VijayanB VijayanB self-assigned this Apr 10, 2021
@VijayanB
Copy link
Member Author

curl http://localhost:9200

3c22fb3bf9b7:opendistroforelasticsearch-1.13.1 2 balasvij$ curl http://localhost:9200
{
  "name" : "integTest-0",
  "cluster_name" : "integTest",
  "cluster_uuid" : "NlduXDMESa6u9V2M6UXYNA",
  "version" : {
    "number" : "7.10.3-SNAPSHOT",
    "build_type" : "tar",
    "build_hash" : "f7a7b80fbad8a91403771941b26e246f7b8108d1",
    "build_date" : "2021-03-26T22:11:35.988476Z",
    "build_snapshot" : true,
    "lucene_version" : "8.7.0",
    "minimum_wire_compatibility_version" : "6.8.0",
    "minimum_index_compatibility_version" : "6.0.0-beta1"
  }
}

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Overall the changes look good to me.
Few minor comments.

build.gradle Outdated
Comment on lines 18 to 19
es_version = System.getProperty("es.version", "7.10.3")
es_group = "org.opensearch"
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit, can we rename es to opensearch

Suggested change
es_version = System.getProperty("es.version", "7.10.3")
es_group = "org.opensearch"
opensearch_version = System.getProperty("es.version", "7.10.3")
opensearch_group = "org.opensearch"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

build.gradle Outdated
Comment on lines 79 to 80
//apply plugin: 'elasticsearch.esplugin'
//apply plugin: 'elasticsearch.rest-test'
Copy link
Member

Choose a reason for hiding this comment

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

Looks like an unintended comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Thanks.

@VijayanB
Copy link
Member Author

Will rename variables in next PR. Would like to make sure build works with OpenSearch as upstream.

Copy link
Member

@jmazanec15 jmazanec15 left a comment

Choose a reason for hiding this comment

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

Will file paths be updated in a followup PR?

Also, does Cmake file need to be updated in this PR? https://github.com/opensearch-project/k-NN/blob/main/jni/CMakeLists.txt

gradle.properties Outdated Show resolved Hide resolved
@VijayanB
Copy link
Member Author

Will file paths be updated in a followup PR?

Also, does Cmake file need to be updated in this PR? https://github.com/opensearch-project/k-NN/blob/main/jni/CMakeLists.txt

I believe that library or path should nat have impact since we are just updating only the upstream. if not, can you let me know where are we using absolute path instead of relative?

Copy link
Member

@vamshin vamshin 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants