Implement a backup tool

• Key: JCR-442
• Type: New Feature
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Jukka Zitting
• Reporter: Jukka Zitting
• Component/s: jackrabbit-core, jackrabbit-standalone
• Fix Version/s: 1.6.0

Description

Issue for tracking the progress of the Google Summer of Code project assigned to Nicolas Toper. The original project requirements are:

"Implement a tool for backing up and restoring content in an Apache Jackrabbit content repository. In addition to the basic content hierarchies, the tool should be able to efficiently manage binary content, node version histories, custom node types, and namespace mappings. Incremental or selective backups would be a nice addition, but not strictly necessary."

Comments

[Nicolas Toper]

It is my first patch ever ;) Please be indulgent. Tell me if I did it right.

[Tobias Bocanegra]

some thoughts:
- why needs BackupConfig.init() the repositoryimpl ? i would do a Config.getBackup(repo) instead.
- you don't need to call super() in the Backup class, since it extends from Object.
- if config creates backup, why do you need to pass config to save() ? (and why not passing one to restore?)
- if config.getBackup() returns a Backup, why does RepositoryImpl.getBackupRepository() return a BackupRepository ?

[Tobias Bocanegra]

btw: patch is perfect :-) (except, that it could be relative to the jackrabbit directory)

[Jukka Zitting]

Looks good. Once you're done fixing the issues mentioned here and on the ML, I'll be happy to merge the changes to the svn trunk.

[Nicolas Toper]

I have updated the code as seen through the mailing list. I am more available now that most of my exams are over (only one remaining on Friday - crossing fingers).

If everybody's OK with this work. I will continue working on the methods body (see methods started but commented).

I am waiting for your feedback :)

[Jukka Zitting]

Thanks for the update, looks good. There's some small issues, but I think it's better to let the source evolve instead of fixing the details at this point.

One relatively major thing though before I merge this... Could you rename also ConfigBackup to BackupConfig? The current name indicates "configuration backup" rather than the intended "backup configuration".

[Nicolas Toper]

ConfigBackup is now called BackupConfig.

It is my first accepted patch on Apache Jackrabbit :)

[Jukka Zitting]

Thanks, merged in revision 418655.

You'll probably get a ton of conflicts when you update, as I made some checkstyle adjustments to your code (replaced tabs with spaces, etc.). In future, use spaces for tabs and the HEADER.txt without even whitespace changes (using an exact copy allows easy automated checking) as the license header.

[Nicolas Toper]

Deleting the backup package and restoring RepositoryImpl to its previous configuration.

[Nicolas Toper]

LaunchBackup tested and completed. (issue with Eclipse and SVN so please tell me if you have a problem importing the patch)

[Nicolas Toper]

Jackrabbit patch with 2 classes for ConfigurationParser. I couldn't test the class since Jackrabbit does not currently compile through Maven.

In theory no problem, since I just moved some methods from one class to the other (and add super where needed).

[Jukka Zitting]

Applied the patches that removed the backup classes from core (patch.txt [12336377]) and added them to the contrib/backup subproject (patch.txt [12336687]) .

The latter patch required some tweaking as it didn't compile out of the box (see BackupConfig.create(InputSource, String)). Please check that the project compiles before sending a patch, normally a patch that doesn't compile would be rejected without further consideration. And (as already discussed in private) please also use spaces for indentation instead of tabs.

[Jukka Zitting]

> Jackrabbit patch with 2 classes for ConfigurationParser.

This patch (jackrabbit-1.patch.txt) is not valid. It seems to remove the entire ConfigurationParser class but then reference it from a partially modified RepositoryConfigurationParser class (that doesn't even exist yet in svn). Please review your setup and try recreating the patch.

[Nicolas Toper]

- XML configuration file is correctly parsed and interpreted by the Backup tool.
- code compiles and is tested
- Indentations is composed of spaces and not anymore tabs.

[Nicolas Toper]

- The new correct patch (I don't try to delete and recreate the same file within the same patch)
- No tabs :p

[Jukka Zitting]

> The new correct patch (I don't try to delete and recreate the same file within the same patch)

I see what you're trying to do, but the patch still tries to modify the non-existent RepositoryConfigurationParser class instead of correctly adding it. Did you start it as a copy of ConfigurationParser? I tried doing the same and was able to apply the patch without problems.

Still, I didn't yet commit the change, as there are two issues I'd like resolved:

a) Make RepositoryConfigurationParser a subclass of ConfigurationParser and use inheritance to access the protected methods from ConfigurationParser.
b) Remove the createSubParser() method from the ConfigurationParser base class, it's only needed by RepositoryConfigurationParser.

[Nicolas Toper]

a) Make RepositoryConfigurationParser a subclass of ConfigurationParser and use inheritance to access the protected methods from ConfigurationParser.
b) Remove the createSubParser() method from the ConfigurationParser base class, it's only needed by RepositoryConfigurationParser.
c) Same issue as always for those patches: I delete and recreate the class. I don't know if it's solved but we won't have anymore this problem anyway for the next patches.

[Jukka Zitting]

> Same issue as always for those patches: I delete and recreate the class. I don't know if it's solved but we won't have anymore this problem anyway for the next patches.

Now it's even worse. This patch (patch-jackrabbit-060718.txt) would totally remove the ConfigurationParser, and it still doesn't handle the RepositoryConfigurationParser class properly. I think I got your idea though, I'll recreate the changes manually to get us forward. It's perhaps better to avoid using the Eclipse file copy and move operations while working via patches.

[Jukka Zitting]

Committed the ConfigurationParser changes in revision 423267.

[Nicolas Toper]

Patch for the backup tool:

1/ Parsing of the configuration file is over
2/ Instanciation of the backup manager
3/ First draft of the ZipBackupIOHandler.

The code is still a work in progress. All your comments are greatly appreciated.

[Jukka Zitting]

Committed patch-backup-060719.txt in revision 423575.

Some issues to consider:

* The SizeException and RepositoryConfigBackup classes do not have the correct license headers. Please add them.
* What is the SizeException used for?
* You still have tabs in the code. Check your Java / Code Style / Formatter settings. I'm using the "Java" settings that has the Tab Policy of "Spaces only".
* Consider renaming ManagerBackup (that backs up the manager) to BackupManager (that manages the backup). See the naming discussions we had earlier.
* Could you come up with a simple test case that sets up a dummy repository and launches the backup tool on it? For now it doesn't need to actually do anything useful, just to check that the code runs in addition to it compiling.

Other than that, keep up the good work! It's nice to see things forming up.

[Nicolas Toper]

Update to JR core in order to achieve Backup. Here are the modifications:
- RepositoryImpl: getNodeTypeRegistry is now public + Properties loadRepProps() + public String[] getWorkspaceNames() public getSystemSession()
- added in RepositoryImpl WorkspaceConfig getWorkspaceConfig(String workspaceName)
- NodeTypeRegistry: added method public NodeTypeDef[] getRegisteredNodesTypesDefs()
- Any tabs issues? (I found some in some files I didn't update so I didn't dare to replaceAll "regexpically")
- class SystemSession is now public

Maybe I have put too many classes in public. Please tell me...

[Nicolas Toper]

"first sizable patch". Not everything is working yet...

- solved tab issue
- renamed ManagerBackup as BackupManager
- no more SizeException
- added missing ASF license headers
- test case added in src/test: some path are hardcoded: how should I generify the test case? Which other ones would you want?
- Backup nearly over. (I am starting to work on the restore as soon as we all agree on the backup.) A lot of todo are still there :p
- I have seen a lot of sanityCheck in the current code. Should I put also a lot of them in mine :p?
 
A list of question is coming by email :)

Thanks for your help
Nico

[Nicolas Toper]

A few remarks I forgot to write: I was outside of Internet connection for a few days, this is why this patch is so "big". You will find the current backup structure.

Not everything is working and is final yet.

Some code still needs refactoring and to be commented.

Can you please tell me which test case should I write? Do you have any idea already?

Can you please check the way I am doing the backup is fine.

Soon, we will be able to backup things up :)

[Jukka Zitting]

Some comments:

a) You can get the NamespaceRegistry and NodeTypeManager (and through it, the NodeTypeRegistry) instances and the list of available workspaces through Session.getWorkspace(). See the JSR-170 javadocs.

b) It might be better to add the getWorkspaceConfig() method to WorkspaceImpl and access it through the system session for that workspace.

c) I'm a bit worried about publishing the SystemSession, but this is likely required for the backup and also some other external efforts like the AccessManagers. We need to check that publishing the system sessions won't cause any trouble.

d) I think the parsePersistenceManagerConfig() method belongs in RepositoryConfigurationParser instead of the ConfigurationParser base class.

e) Do you really need to add the getRegisteredNodesTypesDefs() method? I think it would be nicer if you could collect the registered node types using the existing methods, for example in NodeTypeManager.

f) The patch shows an extra newline in NodeTypeWriter. Try to avoid introducing such changes when you make no other changes to the class. It just adds cruft to the patch and if I were to commit the changes as-is, it would make a new svn version of the file even though nothing really changed.

Could you please fix issues a,b,d,e,f before I can commit these changes.

> solved tab issue

Still seeing some tabs, for example in your getWorkspaceConfig() and getRegisteredNodesTypesDefs() methods (patch-jackrabbit-060725.txt).

> renamed ManagerBackup as BackupManager
> no more SizeException
> added missing ASF license headers

OK, thanks.

> test case added in src/test: some path are hardcoded: how should I generify the test case?
> Which other ones would you want?

This is a good start. You might want to turn the hardcoded paths to command line options and provide reasonable relative paths as defaults. Some notes:

* TestBackup is missing the license header
* Rename the class to BackupTest to comply with the naming in Jackrabbit. I suppose yuor naming issue has roots in the way the French language works, AFAIK you combine words in the opposite order than in English. :-)

> I have seen a lot of sanityCheck in the current code. Should I put also a lot of them in mine :p?

If there's a possibility that your code gets into an invalid state. The sanityChecks in Jackrabbit mostly check whether a repository or a session has already been closed. I don't think a similar case appears with the backup system.

[Nicolas Toper]

Patch including (nearly) all corrections asked by Jukka. See comments please.

[Nicolas Toper]

New patch. Implements corrections asked by Jukka. I changed a lot less methods

[Nicolas Toper]

Hi Jukka,

Here are my answers :p

a/ Done. Maybe I should put in the constructor of Backup the session since it's shared by a few classes. What do you think? I had to look for the default workspace to get a System Session. Can we assume it's always there?

b/ It's actually already the case. I just needed to cast Workspace to WorkspaceImpl. I corrected the code on my side.

c/ Maybe we should add an external tool session instead of sharing a session with JR "internals". What do you think?

d/ This is what I did first, but I use it in BackupConfigurationParser and it is already used in RepositoryConfigurationParser. It is why it's there. (for now I don't use the PM but this might change). I suggest to leave a TODO on this point and see if it's still needed there at the end of the project. Do you agree?

e/ At first I put it in my backup class and then I thought it could be useful to others. It's why it's there. As you can see, I can put it in my backup class if needed but this functionnality can be needed also by other application. What do you think?

f/ Done.

g/ The test case is renamed with the right header.

h/ I have checked the tabs issue *also* in my Jackrabbit patch :p

[Jukka Zitting]

Thanks for the update!

> c/ Maybe we should add an external tool session instead of sharing a session with JR "internals". What do you think?

I'd go for that unless there is something special in the SystemSession class that's needed for the backup.

> d/ This is what I did first, but I use it in BackupConfigurationParser and it is already used in RepositoryConfigurationParser.
> It is why it's there. (for now I don't use the PM but this might change). I suggest to leave a TODO on this point and see if it's
> still needed there at the end of the project. Do you agree?

Not really. Code reuse should be the result of the design, not the other way around. I think it's better for now if you just duplicate the parsePersistenceManagerConfig() method in BackupConfigurationParser instead of pushing it higher, especially since the method is so small.

> e/ At first I put it in my backup class and then I thought it could be useful to others. It's why it's there. As you can see,
> I can put it in my backup class if needed but this functionnality can be needed also by other application. What do you think?

OK. I think it's better to keep the functionality in the backup contrib for now and move it to core when we find another use case. It's another case of code reuse driving the design, which I'm a bit worried about. I don't think it's worth it to expand the public interface of the class with a utility method just for a single client that can achieve the same result using the other methods.

Could you still fix these issues (or counter my arguments :-) before I commit your changes?

[Nicolas Toper]

Hi,

The first part of the project is completed!!!

Backup is working just fine :)

I am posting all patches ASAP. I am waiting for your great comments.

Next steps: the restore operation + extensive testing + some clean up (see TODO tagged items in the code)

[Nicolas Toper]

Working backup operation + relevant test case.

No new classes added.

A lot of issues remain: mostly documentation and code cleaning. They are tagged as TODO in the source code.

[Nicolas Toper]

Jackrabbit patch to support backup operations.

- loadRepProps() in RepositoryImpl is now public
- A new method exportSystemView has been added. The new method has one more parameter: noJcrSystem. It filters out of the export the /jcr:system subtree. Some other classes have been slightly altered to support it. I hope this won't break anything, but please double check.

It is really a pleasure working with you on this project.

[Jukka Zitting]

Committed patch-backup-060728.txt in revision 426435 with some modifications:

* Made the paths in BackupTest relative

* Use Repository.getDescriptorKeys() and Repository.getDescriptor(String) to access repository properties instead of using the internal loadRepProps method. Thus, no need to modify RepositoryImpl.

* Use a SysViewSAXEventGenerator subclass that skips /jcr:system in WorkspaceBackup. Thus, no need to modify SessionImpl and the SAXEventGenerators.

* Depend on RepositoryConfigurationParser for the PERSISTENCE_MANAGER_ELEMENT constant in BackupConfigurationParser. It's a bit ugly, but avoids the need to have the constant in ConfigurationParser. Thus, no need to modify the ConfigurationParsers.

These modifications remove all needs to modify jackrabbit-core, so I won't be applying patch-jackrabbit-060728.txt. :-)

I ran the backup test and received a nice backup ZIP file. Looking good so far!

[Nicolas Toper]

Jackrabbit patch to core to allow restore. 2 updates:

- Added to NamespaceRegistryImpl this method public boolean isRegistered(String prefix). I need to avoid launching of an exception.

- Added to RepositoryImpl, public void setRepProps(Properties props) and public void setRootNodeId(NodeId ni) to restore correctly the repository.

[Nicolas Toper]

Patch to the Backup tool.

Restore operation is starting to work: you can restore all workspaces and repository with NodeType and Namespace.

Next step: restore Node Version Histories. If you have any idea please, tell me :)

A lot of clean up to do and a few issues are still pending (see TODO tags).

Nico

[Jukka Zitting]

Thanks again, great progress! Some issues until I commit your changes:

1) I think it's better for now to stick with the standard NamespaceRegistry methods for checking the existence of a namespace, even at the inconvenience of having to catch an exception. It seems like your backup tool ends up being quite general, so it might also be useful for other JCR implementations! Thus, the fewer jackrabbit-specific methods you use, the better.

2) It's better to use the namespace URI than the prefix when checking for namespace existence. The prefix is just a shorthand for the namespace URI, so it shouldn't be a problem if the namespace already exists with a different prefix.

3) I think the setRepProps() method should be named setDescriptors() to avoid introducing internal concepts at the public method layer. We might in future decide to manage the repository descriptors as something else as a properties hash, so the setDescriptors() name should better stand the test of time.

4) The setDescriptors() method should ideally have some kind of an access check that only permits the administrator to make those changes. I'm not sure if the access control framework can do that yet, so you can just leave a TODO on it for now.

5) The root node ID issue is a bit tricky. Normally it's the same (cafebabe-cafe-babe-cafe-babecafebabe) in all repository instances, so there shouldn't be any need to backup/restore it, but then Jackrabbit uses a file to store the root ID so it's still kind of configurable. However, I think that changing the root node ID of a repository may cause some unexpected issues, so it's probably better not to backup/restore it.

[Jukka Zitting]

... one more thing:

6) You should set the RepositoryImpl.repProps properties in setRepProps/setDescriptors in addition to storing the given properties. This way the restored descriptors will become active immediately and will not be overridden when the repository is shut down.

[Marcel Reutegger]

Regarding repository descriptors, I'm not sure if it makes sense to actually try to restore them. Most of those descriptors are tightly coupled with the internals of an implementation and it's capabilities and are therefore read-only. e.g. whether versioning is supported, etc.

[Jukka Zitting]

Marcel:
> Regarding repository descriptors, I'm not sure if it makes sense to actually try to restore them.
> Most of those descriptors are tightly coupled with the internals of an implementation and it's
> capabilities and are therefore read-only. e.g. whether versioning is supported, etc.

That's true, but the descriptors can also be used to convey custom information, for example the name and contact details of the repository administrator. Jackrabbit doesn't directly support this at the moment, but I think it would be a nice feature to add at some point. Then backing up and restoring that data would make a lot of sense. In fact the setDescriptors() method even effectively implements this feature, now we just need to come up with some semi-standard descriptor keys.

Of course "restored" descriptor should never override the standard descriptors, but Nicolas' implementation already takes care of this by calling setDefaultRepositoryProperties().

[Stefan Guggisberg]

i share marcel's concerns regarding restoring descriptors,
therefore -1 for a setDescriptors() method on Repository

[Nicolas Toper]

- A few classes has been cleaned up and are ready for production (Until BackupManager not included)

- There is still the NodeVersionHistories.restore() unfinished. Eventually, I will unprotect the NodeDef and protect them before going out of the methods (the easier for me I feel).

- I have a bug in WorkspaceBackup.restore() but I can't seem to find it: no content is restored, no error is given. Can you help me on this?

[Jukka Zitting]

Committed patch-060808-backup.txt in revision 429606. Note that I changed the Java 5 String.contains() call to String.indexOf() to make it work with 1.4.

> I have a bug in WorkspaceBackup.restore() but I can't seem to find it: no content is restored, no error is given. Can you help me on this?

Seems correct based on a quick review. Perhaps some debug logging could help?

[Nicolas Toper]

Hopefully, this is one of the last patches for this project :)

Everything is working but the importXML method in the VersionManager hasn't been finished yet (so not committed to core) so the node version history restore is not working (and actually raises an exception since it looks for it). If you need the tool already, please deactivate the node version history restore for now.

The code still needs clean up and documentation but the meat of this contrib project is here.

ETA: Monday.

Next steps: finish the importXML method (currently), clean up (trailing spaces mostly), document (readme, wiki page), test + bug patches and requirements for a version 2.

Some ideas have already been put for a V2 on the wiki page. I plan to start working on it after Google SoC and when we would have gathered enough users feedback. Please feel free to already put your ideas for a v2 and of course, soon, please test the backup/restore.

[Nicolas Toper]

This code contains for now the backup tool with working restore and backup. To currently, it needs a patch on Jackrabbit's core.

We are currently discussing on the ML those issues to know if I should refactor some parts.

Please have a look at the wiki if you would like to know more about this tool.

http://wiki.apache.org/jackrabbit/BackupTool

[Nicolas Toper]

This is the proposed path to the core for the backup tool.

Sorry for the 4 patches but some Gremlins have invaded my computer and created a lot of problem with SVN.

Here are the modifications I propose:

- Update to PropInfo.
Please see relevant thread on ML.

- Update to VersionManagerImpl and to RepositoryImpl.
In order to restore the NodeVersionHistories, I need to gain access to the VersionManagerImpl persistence (through a getter) manager. For this, I need to put the getVersionManager() of RepositoryImpl in public (I couldn't use the one from SessionImpl since there can be a XAVersionManager and the cast to VersionManagerImpl doesn't work in this case)

- Add a NodeVersionHistoriesUpdatableStateManager
It is a specific UpdatableStateManager designed specifically to update the NodeVersionHistories. It is heavily based on the LocalItemStateManager.

I have to be able to connect the imported NodeState so I had to put this class in the o.a.j.core.state package and couldn't put it in the backup one. (connect is protected to state and it is *really* a bad idea to put in public).

The idea was to be able to work with a BatchedItemOperation class so to keep as much as possible its original semantic.

I am sure there is a better way to handle those needs instead of updating the core but I didn't find any yet.

As soon, as we all agree, I will update the relevant part of my code (in backup and in this patch) and document them properly.

Cheers,
Nico
PS I am cleaning up my SVN so next time, I can post one patch for all.

[Nicolas Toper]

The good archive.

[Nicolas Toper]

Since we cannot update this issue, maybe we should close it?

[Nicolas Toper]

   [[ Old comment, sent by email on Fri, 4 Aug 2006 17:48:34 +0200 ]]

It seems two people out of one, want to not backup the descriptors. Is it
the same for the root node?

If so, I'll update the code not to backup those data.

Nico



--
a+
Nico
my blog! http://www.deviant-abstraction.net !!

[Jukka Zitting]

I'm taking over this issue to implement the repository backup and migration tool we need to have in Jackrabbit 1.6 so we can better handle upgrades to the upcoming Jackrabbit 2.0 release.

[Jukka Zitting]

The RepositoryCopier tool in jackrabbit-core and the backup options in jackrabbit-standalone now provide a simple mechanism for backing up or migrating repositories. Resolving as fixed for 1.6.0!