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 batch replay #4108

Merged
merged 5 commits into from
May 5, 2020
Merged

Fix batch replay #4108

merged 5 commits into from
May 5, 2020

Conversation

cjcolvar
Copy link
Member

@cjcolvar cjcolvar commented May 1, 2020 •
edited
Loading

Replay complete testing and reset to pending both used a different means of making the payload than registering a new entry.

Fixes #4109 .

@cjcolvar
Fix replay
3fb4dba 
Replay complete testing and reset to pending both used a different means of making the payload than registering a new entry.
@cjcolvar cjcolvar changed the title Fix replay Fix batch replay May 1, 2020
@cjcolvar
Copy link
Member Author

cjcolvar commented May 1, 2020 •
edited
Loading

The test that is failing is not the one I added. I'm not getting the failure locally and I haven't been able to deduce why it is failing in circleci yet. I'll try to debug this more through ssh to running containers but what I'm seeing right now is instead of an admin collection being created a media object is which breaks everything.
I was able to resolve this by creating a collection and letting it generate it's own id then passing that into the batch entry payload.

@cjcolvar cjcolvar force-pushed the cjcolvar-patch-1 branch from 2e44317 to ca7979b Compare May 4, 2020 15:04
@cjcolvar cjcolvar marked this pull request as ready for review May 4, 2020 15:32
cjcolvar
cjcolvar commented May 4, 2020
View reviewed changes
lib/avalon/batch/ingest.rb Outdated

# Case 1, if there is a payload change and the item never completed, reset to pending
# Also reset to pending if there is an error
reset_to_pending(previous_entry, entry) if !previous_entry.complete || previous_entry.error

# Case 2, if there is a published media object we cannot replay
unless previous_entry.media_object_pid.nil?
mo = MediaObject.find(previous_entry.media_object_pid)
mo = MediaObject.find(previous_entry.media_object_pid) if MediaObject.exists?(previous_entry.media_object_pid)
Copy link
Member Author

@cjcolvar cjcolvar May 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I added this guard to avoid a NotFoundError which would have broken the whole batch registration process, but now I'm thinking that this should be handled in a separate PR since it might include more code changes and need a test.

I would think that if the batch entry has a media object id but the media object can't be found then the batch entry should probably be reset_to_pending so that it will be recreated. @joncameron Do you have an opinion on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and removed it so this PR can go ahead and be merged, but I think it would be good to create a follow-up issue or PR so it doesn't get lost.

@cjcolvar cjcolvar merged commit 9ffc377 into develop May 5, 2020
@cjcolvar cjcolvar deleted the cjcolvar-patch-1 branch May 5, 2020 21:56
@cjcolvar cjcolvar mentioned this pull request May 7, 2020
Open
@joncameron joncameron mentioned this pull request Jun 9, 2020
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phuongdh phuongdh phuongdh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cjcolvar @phuongdh