WIP api: fix recid reservation in PidStore #40
Conversation
kaplun
force-pushed
the
having-recid
branch
from
1c4fae1
to
25ce23a
Compare
|
@egabancho do you have better proposals? |
jirikuncar
changed the title
|
@kaplun for sake of easier understanding of PR please create meaningful commit messages and PR titles. Thank you! PS: As you are part of triage team please assign proper milestone and assignee. |
|
I can't think about a better alternative right now |
|
@jirikuncar what about what I am trying to implement. Does it make sense to you? (albeit it is still failing...) |
|
Just asking because in the old times of BibUpload it took us many moons before deciding on implicit behaviors (such as forcing recids, allowing singletons, etc.) :) |
|
Actually I was thinking about what our old beloved BibUpload would do, I think it would fail if we try |
|
Exactly: that is the only drawbacks. No further unexpected harm. I personally believe --force would not be needed in this case. |
|
@kaplun you need to include |
| pid = PersistentIdentifier.create('recid', pid_value=None, | ||
| pid_provider='invenio') | ||
| pid.reserve() | ||
| record['recid'] = int(pid.pid_value) | ||
| elif not PersistentIdentifier.get('recid', pid_value=recid): |
There was a problem hiding this comment.
Manipulating PIDStore for existing recids might be a dirty business. We should discuss it in wider circle. (cc @inveniosoftware/triagers)
If you are inserting a record with recid, then this recid have been reserved beforehand, hence this elif branch should not happen.
IMHO we can perform this check after insert asynchronously in "checker" module. I see it useful only for initial migration otherwise it will just slow down the normal insertion process.
There was a problem hiding this comment.
Manipulating PIDStore for existing recids might be a dirty business. We should discuss it in wider circle. (cc @inveniosoftware/triagers)
Just to clarify for everybody: this is the use case where incoming record is specifying a recid, which does not exist yet, neither in bibrec nor in pidstore.
In our scenario this is not going to happen only upon first migration, but for an interim period while records are going to be still created on the legacy service and then sent over to the labs one.
IMHO we can perform this check after insert asynchronously in "checker" module. I see it useful only for initial migration otherwise it will just slow down the normal insertion process.
In this case, wouldn't the record then be inserted with a different recid than the one available in the metadata? Then the checker should not/can not amend back pidstore/bibrec, IMHO.
There was a problem hiding this comment.
In our scenario this is not going to happen only upon first migration, but for an interim period while records are going to be still created on the legacy service and then sent over to the labs one.
You should use different provider for your PersistentIdenfiers or force your legacy installation to use labs for reserving recids.
There was a problem hiding this comment.
You should use different provider for your PersistentIdenfiers
That sounds like the most correct usage of PidStore indeed. In this case though records will receive different recids on Labs than on legacy. And this might have many implications in all the relations etc.
force your legacy installation to use labs for reserving recids.
Maybe this is the least painful one. That assumes there is a service running on Labs that wraps the recid provider of invenio-records, correct?
kaplun
force-pushed
the
having-recid
branch
from
25ce23a
to
a50bf85
Compare
| @@ -39,4 +39,5 @@ PACKAGES = [ | |||
| 'invenio_formatter', | |||
| 'invenio_upgrader', | |||
| 'invenio_base', | |||
| 'invenio_pidstore', | |||
There was a problem hiding this comment.
invenio_base should be the last package.
* In case incoming record contains a non existing recid, preallocate the recid in the PidStore. (closes inveniosoftware#39) Signed-off-by: Samuele Kaplun <samuele.kaplun@cern.ch>
kaplun
force-pushed
the
having-recid
branch
from
a50bf85
to
e5d17be
Compare
|
|
This has been supersede by new Invenio-Records and new Invenio-PIDStore |
This is a first tentative to address #39, but it fails with a different error.