4605 rsync upload lock issue #4697
Conversation
|
Functionality looks good - locking consistent with UI/API dataset creation and retrieving transfer script. Some duplications appear to be duplicated (3x notification on checksum verification success, 2x on checksum verification failure) - these might be due to misconfiguration on my part, and might be out of scope for 4605. Duplicate notifications (success and failure) do not appear for "normal" users, so shouldn't be an issue. |
| @@ -166,6 +167,13 @@ public void writeItems(List list) { | |||
| jobContext.setExitStatus("FAILED"); | |||
| return; | |||
| } | |||
| DatasetLock dcmLock = dataset.getLockFor(DatasetLock.Reason.DcmUpload); | |||
There was a problem hiding this comment.
Its kind of confusing that a writeItems method is removing a lock and updating the dataset version. is there a reason this doesn't live in the close method?
There was a problem hiding this comment.
TBH my answer is I don't know. Most of that code was written by somebody who's no longer around.
Maybe the idea is that all these sub-tasks - creating the "package" file, then updating the dataset to reflect it (which in turn requires unlocking the dataset) are all parts of one main task, that cannot be considered complete unless all the parts succeed, transaction-like?
There was a problem hiding this comment.
Ahh ok. Thanks for the clarification!
|
@pameyer - is this looking like this extra notifications are new, as in introduced in this PR? |
|
@landreev - the extra (admin only) notifications aren't new in this PR |
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist