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

Alignment and configuration #40

Merged
merged 1 commit into from
Oct 21, 2016
Merged

Alignment and configuration #40

merged 1 commit into from
Oct 21, 2016

Conversation

mjgiarlo
Copy link
Member

@mjgiarlo mjgiarlo commented Oct 19, 2016 •
edited

  • Rename SynchronizedMinter and align its code with the new Minter::Base-based minter structure
  • Add accessor for default minter class to Config
  • Use configured minter class as default in service
  • Sets the default minter class to Minter::Db (rather than Minter::File -- breaking change from past versions)
  • Update README with instructions for overriding the minter and using Minter::File
  • Add #read and #write methods to minter classes
  • Add new rake tasks that can migrate minter state between persistence options (+ documentation for same)
  • Add test coverage for new minter #read/#write methods

@mjgiarlo mjgiarlo changed the title Alignment and configuration [WIP] Alignment and configuration Oct 19, 2016
@mjgiarlo mjgiarlo changed the title [WIP] Alignment and configuration Alignment and configuration Oct 20, 2016
no-reply
no-reply requested changes Oct 21, 2016
View reviewed changes
Copy link
Member

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

Looks good.

Before a 2.0 release, I think there are some updates due to README.md's example code for MyMinter, to use Base.

README.md
@@ -124,7 +136,7 @@ For more information about the format of Noid patterns, see pages 8-10 of the [N

### Custom minters

If you don't want your minter's state to be persisted, you may also pass in your own minter. First write up a minter class that looks like the following:
If you don't want your minter's state to be persisted, you may also write and configure your own minter. First write up a minter class that looks like the following:

```ruby
class MyMinter
Copy link
Member

Choose a reason for hiding this comment

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

class MyMinter < ActiveFedora::Noid::Minter::Base ?

Copy link
Member

Choose a reason for hiding this comment

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

Does the example code need to change, too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, good call.

@no-reply
Copy link
Member

I have a spec refactor and docs branch, with a question about the #read interface: 2202a07#commitcomment-19521500

This is possibly beyond the scope of this PR, but I'd like to resolve it before 2.0.

@mjgiarlo
Copy link
Member Author

@no-reply README updated.

@no-reply
Copy link
Member

@mjgiarlo that looks good. Do you still mean to pull in the test refactor & docs, or should I push forward with this and submit a follow-up PR?

@mjgiarlo
* Rename SynchronizedMinter and align its code with the new `Minter…
6ca6959 
…::Base`-based minter structure

* Add accessor for default minter class to `Config`
* Use configured minter class as default in service
* Sets the default minter class to `Minter::Db` (rather than `Minter::File` -- **breaking change** from past versions)
* Update README with instructions for overriding the minter and using `Minter::File`
* Add `#read` and `#write` methods to minter classes
* Add new rake tasks that can migrate minter state between persistence options (+ documentation for same)
* Add test coverage for new minter `#read`/`#write` methods
@no-reply no-reply merged commit d80c683 into master Oct 21, 2016
@no-reply no-reply deleted the alignment_and_configuration branch October 21, 2016 22:35
@atz
Copy link
Contributor

atz commented Feb 23, 2017

I think it is problematic to have extracted locking to outside the transaction. Locking for update now happens in multiple places, including on every read. Previously only a transaction could block another transaction. Now a read can.

At the very least, some explanation would be appreciated why that is an improvement. read and write! can now block each other: each calls instance which calls MinterState.lock.find_by, so they are not working on the same object, but each has a lock.

read and write! being public (and superficially documented) methods is sorta a problem. I mean, the Base class says read returns a Hash representing the minter state. Both it and the concrete class should be able to be more specific. (E.G., are the keys symbolic, string or mixed? what's this magic JSON marshalling?) Also, since it is public, a consumer might reasonably call read and be unaware that a locking operation is the result.

By my interpretation, read does not need SELECT ... FOR UPDATE for an instance it converts to a Hash, then throws away. Unlike before, no update ever comes for that object! The point of having the transaction was to put the read and the save! on the same object. This commit effectively undoes that.

Lastly, this was a lot of decomposable changes for one commit. 👎

This was referenced Feb 23, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@no-reply no-reply no-reply 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.

None yet
3 participants
@mjgiarlo @no-reply @atz