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

[HOLD] When moab not found on disk, looks for it. If found, updates catalog. #1271

Closed
wants to merge 1 commit into from
Closed

[HOLD] When moab not found on disk, looks for it. If found, updates catalog. #1271

wants to merge 1 commit into from

Conversation

justinlittman
Copy link
Contributor

closes #1139

Why was this change made?

To allow for the movement of moabs among store roots.

Was the usage documentation (e.g. wiki, README, queue or DB specific README) updated?

No.

mjgiarlo
mjgiarlo reviewed Dec 16, 2019
View reviewed changes
Copy link
Member

@mjgiarlo mjgiarlo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am just now writing up a ticket about difficulties I've had working w/ PresCat, specifically with refactoring away from complex mixins, and it occurs to me that this PR increases this codebase's usage of mixins and may make MoabValidationHandler and PreservedObjecthandler more difficult to grok. I wonder if you would consider, or already considered, making MoabMovedHandler a class ("service object" if you will) rather than a mixin. This will reduce the amount of kitchen-sinkism that is already making prescat a bit of a 🐻.

Update: I wrote up an issue about this: #1276 cc: @jmartin-sul

@jcoyne
Copy link
Contributor

jcoyne commented Dec 16, 2019

@justinlittman I agree with Mike, while that one method is much better, I'm having a harder time understanding the mixins. I think making it a class would be the right solution.

@justinlittman
Copy link
Contributor Author

Fortunately, I agree with both of you.

@justinlittman justinlittman force-pushed the t1139-move_moab branch 2 times, most recently from 1e9387a to e7fef50 Compare December 17, 2019 13:30
jcoyne
jcoyne requested changes Dec 17, 2019
View reviewed changes
mjgiarlo
mjgiarlo approved these changes Dec 17, 2019
View reviewed changes
Copy link
Member

@mjgiarlo mjgiarlo left a comment

Choose a reason for hiding this comment

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

Concur with @jcoyne's feedback. Otherwise, looking ready to go!

app/services/moab_moved_handler.rb

##
# Service for handling a Moab that has moved
class MoabMovedHandler
Copy link
Member

Choose a reason for hiding this comment

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

Big improvement. Thank you!

@justinlittman justinlittman requested a review from jcoyne December 17, 2019 18:44
jmartin-sul
jmartin-sul requested changes Dec 18, 2019
View reviewed changes
Copy link
Member

@jmartin-sul jmartin-sul 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 small suggestions/questions to consider, but overall this looks great to me

app/services/moab_moved_handler.rb
version = nil
transaction_ok = ActiveRecordUtils.with_transaction_and_rescue(results) do
version = complete_moab.version
end
Copy link
Member

Choose a reason for hiding this comment

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

taking this opportunity to refresh myself on the use of this wrapper... and my read is that its use here is superfluous (though also harmless). because the only thing that's happening is reading the field of an already loaded ActiveRecord object, there's no set of multiple db operations that need to fail or succeed atomically (so no need for a transaction). and neither of the errors ActiveRecordUtils.with_transaction_and_rescue catches (RecordNotFound and ActiveRecordError) would occur on line 53 anyway (i don't think -- not 100% certain the latter couldn't occur; if it could, maybe the wrapping does make sense, though i think a plain rescue might be clearer).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is required by the unit test that throws a RecordNotFound exception and tests for correct handling. I personally feel that test is a bit excessive, but this is to provide correct handling.

Note that this is all further complicated by the fact that you never know when you're making the first call to complete_moab and that RecordNotFound error might be triggered.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, my read had been that complete_moab was already retrieved because it seemed to be passed in via the constructor, but totally possible i have that wrong.

@justinlittman justinlittman changed the title When moab not found on disk, looks for it. If found, updates catalog. [HOLD] When moab not found on disk, looks for it. If found, updates catalog. Dec 18, 2019
@justinlittman
Copy link
Contributor Author

The more I think about this, the more I realize that this is just one of the cases that needs to be handled to support moabs on multiple storage roots.

What I think we want to do is to collapse CatalogToMoab and MoabToCatalog / PreservedObjectHandler into a single audit. (Otherwise, the handling for fixing problems needs to be put in too many places.)

The single audit would, for a preserved object:

  • For each moab (found on storage across all storage roots):
    • Make sure that there is a complete_moab. If there is not a complete_moab, create a new one with validity unknown.
  • For each complete_moab:
    • Make sure that the complete_moab version matches the preserved object version.
    • Make sure that the moab exists. If the moab does not exist and there is at least one other complete_moab, then delete.
    • Make sure the moab version matches the complete_moab version.

@ndushay
Copy link
Contributor

ndushay commented Dec 18, 2019 •
edited
Loading

How would this work? The checks aren't doing the same thing, and they have diff starting points to get the object to be checked. In M2C you look at the dirs of a storage root to get the objects to be checked; in the other case you look in the db for preserved_objects to be checked. In the M2C check, the moab's validity is thoroughly checked with a class in the moab-versioning gem. (I think. Haven't double checked, but the time to run M2C checks vs. C2M checks would imply this.)

This may benefit from a RT discussion.

@justinlittman
Copy link
Contributor Author

You would still trigger it both ways; however, in each case it runs the same checks.

If I'm reading the code correctly, both M2C and C2M invoke a Stanford::StorageObjectValidator.

Agree about warranting further discussion.

@ndushay ndushay mentioned this pull request May 12, 2020
Closed
@jermnelson jermnelson changed the base branch from master to main January 7, 2021 16:47
@mjgiarlo
Copy link
Member

@justinlittman still worth keeping this guy around or?

@justinlittman justinlittman deleted the t1139-move_moab branch October 24, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianmorley julianmorley julianmorley left review comments

@jcoyne jcoyne jcoyne approved these changes

@mjgiarlo mjgiarlo mjgiarlo approved these changes

@jmartin-sul jmartin-sul jmartin-sul 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.

Handle Moabs that are moved between storage_roots
6 participants
@justinlittman @jcoyne @ndushay @mjgiarlo @jmartin-sul @julianmorley