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

Refactor fixity-related methods #338

Closed
wants to merge 10 commits into from
Closed

Refactor fixity-related methods #338

wants to merge 10 commits into from

Conversation

cbeer
Copy link
Contributor

@cbeer cbeer commented May 5, 2014

The commits below can (and should) be squashed before merging. They've been separated out for understanding all the moving parts, but are probably unable to stand alone.

Most of this work is in preparation for Modeshape 4 / Infinispan 6, which significantly change some of the APIs we've been using (for the better). This is an attempt to refactor the existing code in a way that will work well enough with both what we have now and prepare us for Modeshape 4.

One controversial change might be 962458b - removing self-healing from fixity failures. The way we're currently doing it is not very good, and has no chance of working going forward.

https://www.pivotaltracker.com/story/show/70699932

cbeer added 10 commits May 6, 2014 17:29
This feature was only useful for chained cache loaders on a single host
(e.g. not across clusters, and not for other types of binary stores).
serialization

The content location may be expensive to calculate (some cluster
configurations), is often unnecessary for day-to-day operations, etc
stored

- getFixity() should return a collection of fixity results that provide
  one or more fixity results for all copies of the binary data
- simplify the infinispan distributed executation calls for ISPN-backed
  binary stores
final NodeType primaryNodeType = node.getPrimaryNodeType();
nodeTypesB.add(primaryNodeType);

if (primaryNodeType != null && primaryNodeType.getSupertypes() != null) {
Copy link

Choose a reason for hiding this comment

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

Is primaryNodeType ever null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, no. In our tests, we devote hundreds of lines to mocking out all those chained calls. I don't think it does any harm, and would clean our tests up considerably.

Check out any unit test that calls the NodeRdfContext, for example, e.g. https://github.com/futures/fcrepo4/blob/refactor-fixity/fcrepo-kernel/src/test/java/org/fcrepo/kernel/rdf/impl/HierarchyRdfContextTest.java#L204

I was going to have ReferencesRdfContext extend NodeRdfContext, but all the mocking I needed to do that was just too much trouble.

Copy link

Choose a reason for hiding this comment

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

Fair enough. Have you had a chance to investigate if jcr-mock could help in this situation?
https://github.com/tacitknowledge/jcr-mock

@awoods
Copy link

awoods commented May 8, 2014

I wonder if LowLevelCacheEntry.java can now be removed in this refactoring?

@@ -216,8 +196,7 @@ public RdfStream getFixityResultsModel(final IdentifierTranslator subjects,
* @throws RepositoryException
*/
@Override
public Collection<FixityResult> runFixityAndFixProblems(
final Datastream datastream) throws RepositoryException {
public Collection<FixityResult> runFixityAndFixProblems(final Datastream datastream) throws RepositoryException {
Copy link

Choose a reason for hiding this comment

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

This method should probably be renamed to reflect the fact that we are no longer "fixing problems".

@awoods
Copy link

awoods commented May 10, 2014

@awoods awoods closed this May 10, 2014
@awoods awoods deleted the refactor-fixity branch May 10, 2014 01:09
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

2 participants