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

Collection of RFS tweaks and fixes #743

Closed
wants to merge 3 commits into from

Conversation

chelma
Copy link
Member

@chelma chelma commented Jun 19, 2024

Description

  • Improved an RFS gradle task
  • Updated the main RFS .jar to contain the doc migration class
  • Updated our CDK to run the doc migration entrypoint script
  • Updated the RFS Fargate container to have more storage

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-1748

Testing

  • Tested locally by executing the three phase scripts locally (./gradlew CreateSnapshot:run, ./gradlew MetadataMigration:run, ./gradlew DocumentsFromSnapshotMigration:run) against the 7.10 Docker Compose setup
  • Deployed started the Doc Migration phase script inside the Fargate container in AWS by upping the task count from 0 to 1, ensured that it tried to run. It behaved as expected - setting up the doc migration status document then discovering the snapshot didn't exist in S3 where it was looking. We could fail more gracefully here.
19:12:29.462 INFO  Running RfsWorker
19:12:31.650 INFO  Checking if work remains in the Documents Migration Phase...
19:12:31.654 INFO  Documents Migration not yet completed, entering Documents Phase...
19:12:31.654 INFO  Pulling the Documents Migration entry from the CMS, if it exists...
19:12:34.015 INFO  Documents Migration CMS Entry not found, attempting to create it...
19:12:34.580 INFO  Documents Migration CMS Entry created
19:12:34.581 INFO  Setting the worker's current work item to be creating the documents work entries...
19:12:34.581 INFO  Work item set
19:12:34.581 INFO  Setting up the Documents Work Items...
19:12:35.358 ERROR Documents Migration Phase failed w/ an exception 
com.rfs.worker.DocumentsRunner$DocumentsMigrationPhaseFailed: Documents Migration Phase failed
	at com.rfs.worker.DocumentsRunner.runInternal(DocumentsRunner.java:52) ~[RFS.jar:?]
	at com.rfs.worker.Runner.run(Runner.java:18) ~[RFS.jar:?]
	at com.rfs.MigrateDocuments.lambda$main$0(MigrateDocuments.java:108) ~[RFS.jar:?]
	at com.rfs.common.TryHandlePhaseFailure.executeWithTryCatch(TryHandlePhaseFailure.java:26) [RFS.jar:?]
	at com.rfs.MigrateDocuments.main(MigrateDocuments.java:90) [RFS.jar:?]
Caused by: java.lang.StringIndexOutOfBoundsException: String index out of range: -5
	at java.base/java.lang.String.substring(String.java:1841) ~[?:?]
	at com.rfs.common.S3Uri.<init>(S3Uri.java:9) ~[RFS.jar:?]
	at com.rfs.common.S3Repo.findRepoFileUri(S3Repo.java:60) ~[RFS.jar:?]
	at com.rfs.common.S3Repo.getSnapshotRepoDataFilePath(S3Repo.java:119) ~[RFS.jar:?]
	at com.rfs.version_es_7_10.SnapshotRepoData_ES_7_10.fromRepo(SnapshotRepoData_ES_7_10.java:29) ~[RFS.jar:?]
	at com.rfs.version_es_7_10.SnapshotRepoProvider_ES_7_10.getRepoData(SnapshotRepoProvider_ES_7_10.java:20) ~[RFS.jar:?]
	at com.rfs.version_es_7_10.SnapshotRepoProvider_ES_7_10.getIndicesInSnapshot(SnapshotRepoProvider_ES_7_10.java:34) ~[RFS.jar:?]
	at com.rfs.worker.DocumentsStep$SetupDocumentsWorkEntries.run(DocumentsStep.java:247) ~[RFS.jar:?]
	at com.rfs.worker.DocumentsRunner.runInternal(DocumentsRunner.java:45) ~[RFS.jar:?]
	... 4 more

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • 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.

Copy link

codecov bot commented Jun 19, 2024

Codecov Report

Attention: Patch coverage is 0% with 38 lines in your changes missing coverage. Please review.

Project coverage is 64.51%. Comparing base (b9d2d5f) to head (dd1abd3).
Report is 21 commits behind head on main.

Files Patch % Lines
RFS/src/main/java/com/rfs/MigrateDocuments.java 0.00% 38 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #743      +/-   ##
============================================
+ Coverage     63.89%   64.51%   +0.62%     
- Complexity     1584     1585       +1     
============================================
  Files           223      239      +16     
  Lines          9092     9918     +826     
  Branches        771      771              
============================================
+ Hits           5809     6399     +590     
- Misses         2874     3108     +234     
- Partials        409      411       +2     
Flag Coverage Δ
gradle-test 63.73% <0.00%> (?)
python-test 73.57% <ø> (?)
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@gregschohn gregschohn left a comment

Choose a reason for hiding this comment

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

I took a look earlier at how we build images and what it would take to do it, since I only had about 10 minutes, I put a PR together that's based off of another branch that I was currently working out of. However, all the changes that are necessary should be in this one commit. That would also eliminate the need (pretty sure) to do any CDK change to pickup a new service name or classname.

This PR undoes part of the PR from a couple days ago that was approved to break the DocumentsFromSnapshotMigration project out of the RFS package, setting up the RFS package to a common jar, and not a source of any specific applications.

If you want to revert the earlier work, please do NOT rename RfsMigrateDocuments.java at the moment. It will be much harder to manage merge conflicts and the history, which is changing considerably at this point in time.

@peternied
Copy link
Member

@chelma this PR seems out of date, still want to merge parts of it or close it out?

@chelma
Copy link
Member Author

chelma commented Jul 16, 2024

Irrelevant at this point; closing

@chelma chelma closed this Jul 16, 2024
@chelma chelma deleted the MIGRATIONS-1748 branch October 23, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants