-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fix: correct gradle file function name and comment #795
Conversation
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? |
Seems the rolling upgrade is failing because now is between the code freeze data and release date. |
Correct |
@@ -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)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the steps:
- pullOpensearchArtifact downloads the whole opensearch artifact with plugins.
- Tasks like pullBwcPlugin copy the plugin directory within the opensearch artifact plugin folder.
- Tasks like zipBwcPlugin zips the plugin directory into a zip file.
- testClusters in restart upgrade and rolling upgrade installs the plugin from zip file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Can someone help attach backport 2.x label? |
You can review after merging #834 |
#834 Merged, can someone help rerun CI tests and review? |
Signed-off-by: yuye-aws <[email protected]>
Signed-off-by: yuye-aws <[email protected]>
* fix gradle file function name and comment Signed-off-by: yuye-aws <[email protected]> * update changelog Signed-off-by: yuye-aws <[email protected]> --------- Signed-off-by: yuye-aws <[email protected]> (cherry picked from commit d96f7d1)
* fix gradle file function name and comment Signed-off-by: yuye-aws <[email protected]> * update changelog Signed-off-by: yuye-aws <[email protected]> --------- Signed-off-by: yuye-aws <[email protected]> (cherry picked from commit d96f7d1) Signed-off-by: Martin Gaievski <[email protected]> Co-authored-by: yuye-aws <[email protected]> Co-authored-by: Martin Gaievski <[email protected]>
Description
Correct gradle file function name and comment.
Issues Resolved
Correct gradle file function name and comment.
Check List
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.