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.

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

[WIP] Deprecate Hyrax's Noid functionality #1049

Closed
wants to merge 1 commit into from
Closed

Conversation

jcoyne
Copy link
Member

@jcoyne jcoyne commented May 25, 2017

NOID has been the favored ID generator since Sufia's early days. But there are some issues with NOID:

  1. It may be faster and less error prone to use the default Fedora UUIDs.
  2. Some institutions may favor their own service for generating identifiers.
  3. Adding another mandatory dependency slows the build.

Deprecating NOID integration will enable us to address these issues.

NOTE If you were using noids, you must now explicitly turn them on: config.enable_noids = true. Previously this was the default.

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.

Please leave this PR open until we've had a chance to discuss the ramifications on the May 31st community tech call. Thanks!

@jcoyne
Copy link
Member Author

jcoyne commented May 26, 2017

I'm not sure how to get these tests to run. Travis says: "The repository at projecthydra-labs/hyrax was not found."

@mjgiarlo
Copy link
Member

@jcoyne have you tried pushing a change to the branch?

@jcoyne jcoyne force-pushed the without_noid branch 3 times, most recently from 48951ef to 29fc158 Compare May 26, 2017 14:29
Enable using ActiveFedora::Noid::Model if desired
@@ -500,29 +501,6 @@ def paranoid_edit_permissions
end
end

describe 'assign_id' do
Copy link
Member Author

Choose a reason for hiding this comment

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

This block tests the same stuff as the block starting at line 222

@escowles
Copy link
Contributor

I see reasons to make using Fedora's UUIDs the default (faster, less error prone, etc.) — but I don't see any reason for deprecating NOIDs. I've heard tell of errors minting NOIDs at scale, but have never seen these personally.

@jcoyne
Copy link
Member Author

jcoyne commented May 31, 2017

@escowles It does not deprecate the use of noids, It just deprecates the tight coupling between hyrax and AF-noid. You can still use AF noid by including ActiveFedora::Noid::Model on your model classes.

@hackartisan
Copy link
Contributor

Link to background discussion on this change? I would expect implications for the maintenance and stability of the NOID codebase over time.

@jcoyne
Copy link
Member Author

jcoyne commented May 31, 2017

@HackMasterA I'm not sure I understand this statement:

I would expect implications for the maintenance and stability of the NOID codebase over time.

The only way I can see decoupling components would affect maintenance is that people who are not using those components would not be involved in their maintenance. That seems fair, right?

I'm not sure how stability would be affected in any way.

@jcoyne
Copy link
Member Author

jcoyne commented May 31, 2017

@HackMasterA I don't think there has been any prior discussion. In hyku we had to stop using AF-noid because it was a bottleneck on large imports.

@hackartisan
Copy link
Contributor

Can you say more about what you observed with hyku?

@mjgiarlo
Copy link
Member

We'll be discussing on the Tech call momentarily, for folks who can dial in.

@sethaj sethaj mentioned this pull request Jun 5, 2017
3 tasks
@mjgiarlo
Copy link
Member

mjgiarlo commented Jun 5, 2017

We decided not to move forward with deprecating NOID usage in Hyrax given the community's desire to keep using NOIDs.

@atz
Copy link
Contributor

atz commented Aug 4, 2017

@mjgiarlo so we close this?

@jcoyne
Copy link
Member Author

jcoyne commented Aug 4, 2017

@atz please don't. The feedback from this meeting was that certain parts of the PR were acceptable, but outright deprecation was not. So I'd like to rework it when I come back to Hyrax at some point.

@jcoyne
Copy link
Member Author

jcoyne commented Nov 22, 2017

Working on an approach to this in:
samvera/active_fedora#1295

Which will require:
samvera/hydra-head#425
And:
#2256

@jenlindner
Copy link
Contributor

Where are things at with this? samvera/hydra-head#425 was merged, #2256 had been blocked by two pulls that have been merged. Looks like only samvera/active_fedora#1295 is left, where there's an open question but no opposition, it seems?

@cjcolvar cjcolvar changed the title Deprecate Hyrax's Noid functionality [WIP] Deprecate Hyrax's Noid functionality Apr 25, 2018
@cjcolvar
Copy link
Member

@no-reply @vantuyls This issue has been open for a year. Given that it was decided to not merge this PR as a whole is there a plan to incorporate some of these changes or can this PR be closed?

@jcoyne jcoyne closed this Apr 25, 2018
@jcoyne jcoyne deleted the without_noid branch April 25, 2018 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants