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

irods_sync script fixes and enhancements (plus one other bug fix) #294

Merged
merged 7 commits into from
Oct 8, 2024

Conversation

alanking
Copy link
Collaborator

@alanking alanking commented Oct 4, 2024

Addresses #91
Addresses #92
Addresses #93
Addresses #210
Addresses #293

I'm contemplating adding tests for these subcommands, but it seems a little tricky (hence, draft PR) and there are no existing tests for any of the subcommands other than start. Existing tests are passing.

Also, the two commits referencing #210 (only) can be squashed into one if we like the solution of maintaining a list of stopped jobs. Otherwise, we can drop the second commit because the first one addresses the issue (albeit, not as well, IMO).

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@korydraughn
Copy link
Contributor

Are you looking for feedback on this PR even though it's in draft?

@alanking
Copy link
Collaborator Author

alanking commented Oct 4, 2024

I want to confirm that this is doing what we want before marking as ready for review. Feel free to leave feedback on what you see, but the implementation may change significantly depending on how the requirements settle out.

@korydraughn
Copy link
Contributor

Ok. Will put eyes on this once it's out of draft.

@alanking alanking force-pushed the irods_sync-fixes branch 2 times, most recently from c32360e to 49139e8 Compare October 4, 2024 18:55
@alanking
Copy link
Collaborator Author

alanking commented Oct 4, 2024

I squashed the two #210 commits together since we're pretty into the stopped jobs list. The stopped jobs list now includes all the information shown in the active jobs list subcommand output. I think this is ready for review...

@alanking alanking marked this pull request as ready for review October 4, 2024 18:56
@korydraughn
Copy link
Contributor

Ok. Will get eyes on it.

Copy link

@MartinFlores751 MartinFlores751 left a comment

Choose a reason for hiding this comment

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

A few comments.

irods_capability_automated_ingest/redis_key.py Outdated Show resolved Hide resolved
irods_capability_automated_ingest/redis_key.py Outdated Show resolved Hide resolved
irods_capability_automated_ingest/sync_job.py Show resolved Hide resolved
irods_capability_automated_ingest/sync_job.py Show resolved Hide resolved
irods_capability_automated_ingest/sync_job.py Outdated Show resolved Hide resolved
@korydraughn
Copy link
Contributor

Anything left for this? Are the tests passing?

@alanking
Copy link
Collaborator Author

alanking commented Oct 7, 2024

I'll run the test suite once more for good measure, but these changes mainly affect the sync script rather than the framework, so these should probably be passing. I'll report back with results soon.

I would also like to update the commit message for "Reset Redis handles for stopped jobs", but that's about it for this one.

@korydraughn
Copy link
Contributor

Sounds good. Will wait for the test results before approving.

@alanking
Copy link
Collaborator Author

alanking commented Oct 7, 2024

Tests are passing.

@alanking
Copy link
Collaborator Author

alanking commented Oct 7, 2024

Squashed, tweaked commit message, and added a thing that Black formatter caught.

Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Pound it

This ensures that interrupting watch (or start --progress) does not
display the whole stacktrace and exit with an error code.
This commit adds a new Redis key for ingest job start times. The value
is reset along with the other information related to a job. This allows
monitoring jobs with accurate "elapsed time" reporting.
Adds a new list to the tracked Redis keys:
irods_ingest_stopped_jobs. This tracks jobs which
are manually stopped with the stop subcommand.

Displays a warning message to the irods_sync script's
log output when a job being monitored by start --progress
or watch is stopped.

The list subcommand now shows the list of stopped jobs
and last-known information about them when they were stopped.
This commit adds job information which is available
when monitoring a job to the list subcommand. These
include the job name (already shown previously), total
number of tasks, number of tasks remaining, number of
failed tasks, number of retried tasks, a formatted
timestamp of what time the job started, and the amount
of time elapsed since the job started.

Also aligns watch subcommand output with information
now being shown in list subcommand output.
This commit documents several new irods_sync script features
as well as adds documentation for some of the subcommands
which previously had little to no documentation.

The stop subcommand now adds job names to a new Redis key,
which is being documented.

The list subcommand got some new information to display in
addition to being able to list stopped jobs.

The irods_sync script section has been reorganized as well
so that the parts fit together a little better.
@alanking
Copy link
Collaborator Author

alanking commented Oct 8, 2024

#'d, mergin

@alanking alanking merged commit bde781a into irods:main Oct 8, 2024
@alanking alanking deleted the irods_sync-fixes branch October 8, 2024 01:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants