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

Jump to bottom

Fix: correct gradle file function name and comment #795

Merged
merged 2 commits into from
Jul 22, 2024
Merged

Fix: correct gradle file function name and comment #795

merged 2 commits into from
Jul 22, 2024

Conversation

yuye-aws
Copy link
Member

@yuye-aws yuye-aws commented Jun 17, 2024 •
edited
Loading

Description

Correct gradle file function name and comment.

Issues Resolved

Correct gradle file function name and comment.

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed as 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.

@martin-gaievski
Copy link
Member

So in today's code we're doing everything correctly, just the function name and comment says "unzip" while in fact we're doing "zip" operation?

@yuye-aws
Copy link
Member Author

Seems the rolling upgrade is failing because now is between the code freeze data and release date.

@yuye-aws
Copy link
Member Author

So in today's code we're doing everything correctly, just the function name and comment says "unzip" while in fact we're doing "zip" operation?

Correct

zhichao-aws
zhichao-aws reviewed Jun 18, 2024
View reviewed changes
CHANGELOG.md
@@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
### Features
### Enhancements
### Bug Fixes
- Fix function names and comments in the gradle file for BWC tests ([#795](https://github.com/opensearch-project/neural-search/pull/795/files))
Copy link
Member

@zhichao-aws zhichao-aws Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please provide more details about why we're zip it instead of unzip? For my previous understanding, we fetch the zip archive from the CI website and unzip it to build the cluster.

Copy link
Member Author

Choose a reason for hiding this comment

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

These functions are actually doing zipping instead of unzipping. This PR does not change any code logic and just fix comments and function names to avoid confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my expression maybe not clear enough. My question is, how we're doing the zip? Could you please describe the whole execution process of the zip* or unzip* function?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is used to install plugin with zip, you can check the gradle file in restart upgrade and rolling upgrade.

versions = [ext.neural_search_bwc_version, opensearch_version]
plugin(project.tasks.unZipBwcMlCommonsPlugin.archiveFile)
plugin(project.tasks.unZipBwcKnnPlugin.archiveFile)
plugin(project.tasks.unZipBwcPlugin.archiveFile)

Copy link
Member Author

@yuye-aws yuye-aws Jun 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Here are the steps:

  1. pullOpensearchArtifact downloads the whole opensearch artifact with plugins.
  2. Tasks like pullBwcPlugin copy the plugin directory within the opensearch artifact plugin folder.
  3. Tasks like zipBwcPlugin zips the plugin directory into a zip file.
  4. testClusters in restart upgrade and rolling upgrade installs the plugin from zip file.

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

qa/build.gradle
dependsOn "pullKnnBwcPlugin"
dependsOn "unZipBwcMlCommonsPlugin"
dependsOn "zipBwcMlCommonsPlugin"
from(Path.of(tmp_dir.absolutePath, "opensearch-knn"))
destinationDirectory = tmp_dir
archiveFileName = "opensearch-knn-${neural_search_bwc_version_no_qualifier}.zip"
Copy link
Member

Choose a reason for hiding this comment

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

Do you know where is this archiveFileName used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Used to define file for plugin installation.

Copy link
Member

Choose a reason for hiding this comment

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

I did some research and find that this parameter is inhereted from the "zip" task, and is used to indicate the final file of "zip" task

Copy link
Member Author

Choose a reason for hiding this comment

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

You can check AbstractArchiveTask for gradle: https://docs.gradle.org/current/dsl/org.gradle.api.tasks.bundling.AbstractArchiveTask.html

@yuye-aws
Copy link
Member Author

Can someone help attach backport 2.x label?

@navneet1v navneet1v added the backport 2.x Label will add auto workflow to backport PR to 2.x branch label Jul 19, 2024
@yuye-aws
Copy link
Member Author

You can review after merging #834

@yuye-aws
Copy link
Member Author

#834 Merged, can someone help rerun CI tests and review?

@yuye-aws
fix gradle file function name and comment
247824e 
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
@yuye-aws
update changelog
1c3010d 
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
@zane-neo zane-neo merged commit d96f7d1 into opensearch-project:main Jul 22, 2024
33 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 22, 2024
@yuye-aws @github-actions
Fix: correct gradle file function name and comment (#795)
e45d3b1 
* fix gradle file function name and comment

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

* update changelog

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

---------

Signed-off-by: yuye-aws <yuyezhu@amazon.com>
(cherry picked from commit d96f7d1)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Jul 22, 2024
Merged
martin-gaievski added a commit that referenced this pull request Aug 7, 2024
@opensearch-trigger-bot @yuye-aws @martin-gaievski
Fix: correct gradle file function name and comment (#795) (#840)
c9ae080 
* fix gradle file function name and comment

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

* update changelog

Signed-off-by: yuye-aws <yuyezhu@amazon.com>

---------

Signed-off-by: yuye-aws <yuyezhu@amazon.com>
(cherry picked from commit d96f7d1)

Signed-off-by: Martin Gaievski <gaievski@amazon.com>
Co-authored-by: yuye-aws <yuyezhu@amazon.com>
Co-authored-by: Martin Gaievski <gaievski@amazon.com>
@yuye-aws yuye-aws deleted the Fix/Gradle branch September 10, 2024 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zane-neo zane-neo zane-neo approved these changes

@zhichao-aws zhichao-aws zhichao-aws approved these changes

@heemin32 heemin32 Awaiting requested review from heemin32 heemin32 is a code owner

@navneet1v navneet1v Awaiting requested review from navneet1v navneet1v is a code owner

@VijayanB VijayanB Awaiting requested review from VijayanB VijayanB is a code owner

@vamshin vamshin Awaiting requested review from vamshin vamshin is a code owner

@jmazanec15 jmazanec15 Awaiting requested review from jmazanec15 jmazanec15 is a code owner

@naveentatikonda naveentatikonda Awaiting requested review from naveentatikonda naveentatikonda is a code owner

@junqiu-lei junqiu-lei Awaiting requested review from junqiu-lei junqiu-lei is a code owner

@martin-gaievski martin-gaievski Awaiting requested review from martin-gaievski martin-gaievski is a code owner

@sean-zheng-amazon sean-zheng-amazon Awaiting requested review from sean-zheng-amazon sean-zheng-amazon is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@ylwu-amzn ylwu-amzn Awaiting requested review from ylwu-amzn ylwu-amzn is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@vibrantvarun vibrantvarun Awaiting requested review from vibrantvarun vibrantvarun is a code owner

Assignees
No one assigned
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yuye-aws @martin-gaievski @zane-neo @zhichao-aws @navneet1v