database backed minter #25
Conversation
| @@ -1,35 +1,19 @@ | |||
| require 'noid' | |||
|
|
|||
| module ActiveFedora | |||
There was a problem hiding this comment.
Instead of making changes directly in this file, how would you feel about breaking your changes into a new class (e.g., SynchronizedDatabaseBasedMinter) and renaming this file with its original contents to e.g., SynchronizedFileBasedMinter?
There was a problem hiding this comment.
I think that's what this PR accomplishes, except for renaming this class.
There was a problem hiding this comment.
The DB minter is a new class, @mjgiarlo. I refactored so that we can use common base class between the two. I consider that the most certain and least objectionable portion of this work. I deferred on renaming the existing SynchronizedMinter for back-compat reasons, but if you prefer, I certainly would move it to be something like ActiveFedora::Noid::Minter::Statefile.
The syncedness (of threads in a single system, single process) is part of Base. It does not need to be called out in naming (especially if the name suggests that it is the same problem as multiple processes on multiple systems using potentially multiple connections to the same database).
If you want to give me a thumbs up, I'll pursue renaming the existing minter. It may make sense to defer that to after getting this initial PR merged.
There was a problem hiding this comment.
Yeah, I totally misread what was going on here. Sorry about that.
There was a problem hiding this comment.
@mjgiarlo: rename SynchronizedMinter or no?
There was a problem hiding this comment.
Meh, not necessary now. Thanks for asking.
|
This looks really good to me. Just the two smallish concerns. Then add tests and |
|
Coverage decreased (-5.1%) to 92.941% when pulling a0975d1f7c47f18495c21df145e3c724c20ef925 on db into 2a9ecb2 on master. |
|
Coverage decreased (-5.1%) to 92.941% when pulling afb0e37a4a75e72fc1948b295562001e14f4fd87 on db into 2a9ecb2 on master. |
|
Coverage decreased (-5.1%) to 92.941% when pulling 0d1694c6509453e38cc6f9eda0a7b7f89318a656 on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.6%) to 94.51% when pulling 9378d82d20cf92d8bffc478154e2220008233942 on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.6%) to 94.51% when pulling 24c765799354069096df1a808b8b4089269c56fd on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.6%) to 94.51% when pulling 7c68eb308a316bb251215d9a591911b16e30fb54 on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.6%) to 94.51% when pulling 3d04a05a8b9d6ea4cda26d178cc83c58bc81534e on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.9%) to 94.141% when pulling cd0594378ca12ceb9e3a24b039e2764d1c161776 on db into 2a9ecb2 on master. |
|
Coverage decreased (-1.4%) to 96.711% when pulling 78a47dad7efb3646085326bcb5098a0d7671dae0 on db into 2a9ecb2 on master. |
|
Coverage decreased (-5.3%) to 92.739% when pulling 5caadf25a9df642591f4de40a3760a873a9fcd45 on db into 2a9ecb2 on master. |
|
Coverage decreased (-5.3%) to 92.739% when pulling d6d3b2250a022c301a050c40958c4cc2bc5b50e3 on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.5%) to 94.611% when pulling f0f24241ae38256e65ca4f5f6060f4d269c5a937 on db into 2a9ecb2 on master. |
|
Coverage decreased (-3.4%) to 94.643% when pulling c521dc2018cf9c481c86308370305b9fb1b2ac31 on db into e6d4af4 on master. |
|
Coverage decreased (-1.3%) to 96.765% when pulling 67462f56b2b8625a2a2945b1386b05dff3ca4ef3 on db into e6d4af4 on master. |
|
Coverage decreased (-1.1%) to 96.961% when pulling a99c611d0b42f741377c9099d1f1015b0c777859 on db into e6d4af4 on master. |
|
CI passing except for ruby version where @jcoyne ready for review. Let me know if you want stuff squashed. |
|
Yes go ahead and squash. |
atz
force-pushed
the
db
branch
2 times, most recently
from
d5bffff
to
7997c78
Compare
atz
force-pushed
the
db
branch
2 times, most recently
from
153db03
to
af371e8
Compare
|
Squashed down to 1/3rd. Got rid of the churn. |
Refactor: - extract AF::Noid::Minter::Base class - extend noid::minter - Will be used by DB minter also. This gets rid of the throwaway new minter per call to `valid?`. In fact it gets rid of overriding `valid?` at all. Template method now returns the template object, not the string (though `my_minter.template.template` gets the string, and the Template object itself now stringifies to the string, which is probably more obvious). Rails and sqlite dependencies as part of activerecord-backed minter work.
Also byebug in the same style as used in Sufia (dev/non-CI only). Cleanup generated cruft from spec_helper.
Use built in engine rake capability to copy migrations. Name is ugly, but effective: `active_fedora_noid_engine:install:migrations` Probably should fix in Sufia also. Basic TestApp generator. DB backed Minter class w/ spec noid_options are for passing to constructor. Tests included. Note that :counters are stored as JSON (text) since they are just data, but :random is an object, therefore stored as Marshall-serialized text. Note: Must use binary or write errors out w/ encoding problems. nils instead of empty strings, remove unnecessary variable assignment. The `with_lock` previously approach was invalid (method not defined on class). The pessimistic lock uses `SELECT FOR UPDATE`, so is basically useless in trying to avoid conflict on insert of the first row. We will escape this problem by having a (generator) task that inserts the first row based on namespace. Additional namespaces can be added by the operator, which we thereby can guarantee are uncontested. Provide generator equivalent of `db:seed` for inserting intial row in table based on Config (or command line args). This ensures that pessimistic locking covers all subsequent cases (via `SELECT FOR UPDATE`). Database level uniquenss constraint and model level validation to ensure consistency.
Spec_helper defaults to test env, rake defaults to :ci Let spec_helper require active_fedora Test ruby 2.3.1 also Set a RAILS_VERSION for travis
Looks like we won't need database_cleaner after all, since truncation would only mean we have to rerun our `db:seed` (alternative generator) anyway.
Consolidate the two ActiveFedora::Noid::Minter::Db test files
- Avoid allow_any_instance_of - Stop testing ActiveFedora::Noid::Service here - factor out common cases (like Minter::Db spec) Possible TODO: test both minters w/ shared examples, since they are almost identical.
The strange thing is that this has been happening continuously on Travis in the Sync Minter specs, but not ever locally. I have no reasonable hypothesis why this is the case, but if this innocuous change pleases the CI gods, then FINE.
|
Rebased. |
|
Fixes #20, incidentally. |
1 similar comment
|
👏 Good job! |
|
Can we |
|
Sorry, @atz I thought I merged yesterday. |
|
well done, @atz! |
No description provided.