After a chat with Jukka, we came up with this solution:

• only work with UUIDs, because this is what the PM uses as identifiers and what is displayed in the error log if something is inconsistent

• extend the PersistenceManger interface by a new method: PersistenceManager.checkConsistency(String[] uuids, bool recurse
• implement it only in BundleDbPersistenceManager, as it is now done by checkConsistency(), but handle the new parameters
• keep the existing configuration based consistencyCheck upon startup (needs migration for the new method)

• RepositoryImpl.checkConsistency(String workspace, String[] uuids) which calls PersistenceManager.checkConsistency(uuids, false)
• RepositoryImpl.checkConsistency(String workspace) will check the entire workspace, calls PersistenceManager.checkConsistency(root_uuid, true)
• to clean up the versioning "workspace", pass "jcr:system" as workspace name

• WorkspaceImpl.checkConsistency(String[] uuids) calls RepositoryImpl.checkConsistency(getName(), uuids)
• WorkspaceImpl.checkConsistency() calls RepositoryImpl.checkConsistency(getName())

• only logging during the check, no special return values

Open Problem:

• Doing the consistency check on an active workspace might cause problems with cache coherence and other such stuff. Doing the consistency check only at startup avoids all those issues, but is much less convenient than an API for doing that.

We could simply defer this to the client calling the consistency check to do so only when there is no activity, but it probably has no full control or awareness of that. Another idea would be to "stop" activity, ie. invalidate any transient sessions and clearing all caches.

WDYT?

I missed that the existing configuration-based implementation has two flags:

• consistencyCheck for enabling the basic process and
• consistencyFix for enabling fixes

If consistencyCheck is "true" and consistencyFix is "false", any inconsistencies would only get logged, but no modification on the data will be done.

This could be done by adding a flag to all new methods or by introducing a separate checkAndFixConsistency() set of methods. I'd prefer the first variant to minimize the number of new API methods (currently I have 3 for WorkspaceImpl and 3 for RepositoryImpl already - checkConsistency(), checkConsistency(String[] uuids) and checkConsistency(String[] uuids, boolean recursive)).

Why are the consistency check method needed on RepositoryImpl? Consistency checking a large workspace is a heavy
weight operation that may have performance effects. Shouldn't such an operation only be allowed to an admin user (-> login required)?

> [...] problems with cache coherence and other such stuff.

why exactly is that an issue? the consistency check works on the persistence manager level, which does not know about transient changes?

> to clean up the versioning "workspace", pass "jcr:system" as workspace name

I would rather use the uuid of the jcr:system node and the method checkConsistency(String[] uuids, boolean)

> currently I have 3 for WorkspaceImpl and 3 for RepositoryImpl already - checkConsistency(),
> checkConsistency(String[] uuids) and checkConsistency(String[] uuids, boolean recursive)

checkConsistency(String[] uuids) is a convenience method, right? I'd remove that one.

> Why are the consistency check method needed on RepositoryImpl?

To build a tool that can call this programmatically. The configuration based variant needs a restart and fiddling with workspace.xml

> Consistency checking a large workspace is a heavy weight operation that may have performance effects.

I improved the check so that it allows you to specify single nodes via uuids.

> Shouldn't such an operation only be allowed to an admin user (-> login required)?

Makes sense. How would I check this inside RepositoryImpl?

> > [...] problems with cache coherence and other such stuff.
> why exactly is that an issue? the consistency check works on the persistence manager level, which does not know about transient changes?

Cached bundles that get deleted by the consistency check on the database level will lead to problems. The cache needs to be invalidated. This is still an open issue...

> > to clean up the versioning "workspace", pass "jcr:system" as workspace name
> I would rather use the uuid of the jcr:system node and the method checkConsistency(String[] uuids, boolean)

RepositoryImpl just calls the consistency check on the persistence manager. Therefore you can specify the workspace in the checkConsistency() methods that will select the PM of the chosen workspace. It was natural to select the versioning PM with a special name then.

I am open for another solution. My question at this point: The versioning PM stores all versions for all workspaces, right? If the nodes in the versioning space have backreferences to the workspace nodes, they have to be global?

> checkConsistency(String[] uuids) is a convenience method, right? I'd remove that one.

Yes, the only one really needed is RepositoryImpl.consistencyCheck(String workspace, String[] uuids, boolean recursive, boolean fix). The other methods in RepositoryImpl and WorkspaceImpl are for convenience (entire workspace check is done via uuids=null).

Shall I go for developer convenience or axiomatic, ie. minimalistic approach here?

BTW: my status is that I have a patch ready, I am currently testing it.

>> Shouldn't such an operation only be allowed to an admin user (-> login required)?
>
> Makes sense. How would I check this inside RepositoryImpl?

You can't. That's why one should login first and do the consistency check on WorkspaceImpl only.

> That's why one should login first and do the consistency check on WorkspaceImpl only.

I see, that's why you proposed to use the uuid of the jcr:system node for checking the versioning pm. Didn't see that there is one WorkspaceImpl per session, hence possibly multiple WorkspaceImpl per actual workspace, and not exact on WorkspaceImpl per workspace (jackrabbit's internal architecture is sometimes hard to follow, because of the mix of JCR API and internal impl naming...).

The problem is that WorkspaceImpl has no access to the persistence managers stored in RepositoryImpl (getWorkspaceInfo() and vMgr for versioning). I see only two solutions:

a) have a public method in RepositoryImpl so that WorkspaceImpl can access it -> no restriction to admin user possible
b) introduce a special private interface that is passed to WorkspaceImpl in constructor that allows access to PMs -> larger refactoring of central classes

Jukka and others, WDYT?

I'm sorry, that's probably a misunderstanding. I'm OK with having one or more methods on RepositoryImpl to check consistency as long as they are not public and are only used by WorkspaceImpl.

> I'm OK with having one or more methods on RepositoryImpl to check consistency as long as they are not public and are only used by WorkspaceImpl

That's not possible without some refactoring in the constructors of WorkspaceImpl and probably others. Don't wanna use package-protected visibility as it is easy to workaround in client code by putting the client class into o.a.j.core (although this is a nice hack in some SWT cases ).

The rationale for having the method on RepositoryImpl is that for now the only known safe time to invoke the consistency check is when the workspace in question has not been opened and there aren't any cached entries. If the method was in WorkspaceImpl, you could only call it once the workspace in question has been opened.

How about making the method protected, then you'd need to subclass RepositoryImpl to access that functionality. That would be in line with the proposed solution in JCR-954.

Jukka wrote:
> How about making the method protected, then you'd need to subclass RepositoryImpl to access that functionality.

+1

> How about making the method protected, then you'd need to subclass RepositoryImpl to access that functionality.

Summing up, this would be the best solution, because the login problem is avoided. I would also drop all convenience methods in RepositoryImpl and WorkspaceImpl.

What should be the name of this new RepositoryImpl subclass? Should be the same as in JCR-954.

Some ideas:

AdminRepositoryImpl (my fav)
AdvancedRepositoryImpl
JackrabbitRepositoryImpl
UseAtYourOwnRiskRepositoryImpl

> Alexander Klimetschek commented on JCR-1428:
> --------------------------------------------
>
> What should be the name of this new RepositoryImpl subclass? Should be the same as in JCR-954.

my understanding was that we agreed to not provide a RepositoryImpl subclass in jackrabbit, just provide
protected methods in RepositoryImpl...

>
> Some ideas:
>
> AdminRepositoryImpl (my fav)
> AdvancedRepositoryImpl
> JackrabbitRepositoryImpl
> UseAtYourOwnRiskRepositoryImpl

> my understanding was that we agreed to not provide a RepositoryImpl subclass in jackrabbit, just provide protected methods in RepositoryImpl...

Yes, you are right of course. Was just tempted to do that because I need to access that method in a test case.

Had some trouble to actually trigger a problem in the JCR API with a child node entry pointing to a missing node, but finally found it (the second one):

> session.getRootNode().getNode("fruits").getNode("bananas");

throws a PathNotFoundException (which is IMHO correct)

> session.getRootNode().getNode("fruits/bananas");

throws "javax.jcr.RepositoryException: failed to retrieve state of intermediary node: 39d2c699-0dad-4b27-aabd-0437913b5339: 39d2c699-0dad-4b27-aabd-0437913b5339" (which should be a PathNotFoundException, hence the consistency check + fix)

Caused by: org.apache.jackrabbit.core.state.NoSuchItemStateException: 03679565-9b2b-4602-9030-1ac2e2e324a4
at org.apache.jackrabbit.core.state.SessionItemStateManager.getItemState(SessionItemStateManager.java:178)
at org.apache.jackrabbit.core.HierarchyManagerImpl.getItemState(HierarchyManagerImpl.java:119)
at org.apache.jackrabbit.core.HierarchyManagerImpl.resolvePath(HierarchyManagerImpl.java:266)
at org.apache.jackrabbit.core.CachingHierarchyManager.resolvePath(CachingHierarchyManager.java:132)
at org.apache.jackrabbit.core.HierarchyManagerImpl.resolvePath(HierarchyManagerImpl.java:266)
at org.apache.jackrabbit.core.CachingHierarchyManager.resolvePath(CachingHierarchyManager.java:132)
at org.apache.jackrabbit.core.HierarchyManagerImpl.resolvePath(HierarchyManagerImpl.java:202)
... 23 more

This could also be fixed by simply throwing a PathNotFoundEx in HierarchyManagerImpl.resolvePath(Path path, ItemId id, int next) instead of a RepositoryEx when a NoSuchItemStateEx is catched there. This would be more consistent.

Otherwise there are probably various complicated scenarios when the child node entry in the underlying database "breaks" during runtime and exceptions pop up everywhere. Are there any typical error.log examples?

Patch which adds a protected RepositoryImpl.checkConsistency(), a public PersistenceManager.checkConsistency() and a full implementation in BundleDbPersistenceManager. For 1.3 branch against rev 631607.

Adds a new test case o.a.j.core.persistence.ConsistencyCheckTest that tests the consistencyCheck feature (and gives an example for RepositoryImpl subclassing). For 1.3 branch.

Committed to trunk (with some changes to make the patch apply) in revision 632993.
Committed to the 1.3 branch in revision 632996.

Note that the version in trunk does not yet include the test case as it depends on JCR-1412.

Resolving as Fixed. Thanks!

Patch (against already commited JCR-1428) that improves the error output to print the full jcr path for a node that has broken child entries. Adds a getBundlePath() helper method to BundleDbPersistenceManager.

This is not vital but might help administrators a lot in debugging the error log output (especially when the node uuids are not already known, eg. in a whole workspace check).

Patch that improves the committed ConsistencyCheckTest from a previous patch to reduced debug logging and to shutdown and clean the repository after testing. Repositories are created as temp directories, which is typically /tmp/jackrabbitXXXX on *nix and C:\Temp under windows. The cleanup avoids piling up files in that directory when running the test multiple times.

(Against 1.3 branch)

Merged to the 1.4 branch in revision 650354.