[ http://issues.apache.org/jira/browse/JCR-442?page=comments#action_12423562 ] Jukka Zitting commented on JCR-442: -----------------------------------
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. > Implement a backup tool > ----------------------- > > Key: JCR-442 > URL: http://issues.apache.org/jira/browse/JCR-442 > Project: Jackrabbit > Issue Type: New Feature > Reporter: Jukka Zitting > Attachments: jackrabbit-1.patch.txt, patch, patch-backup-060716.txt, > patch-backup-060719.txt, patch-backup-060725.txt, > patch-jackrabbit-060716.txt, patch-jackrabbit-060718.txt, > patch-jackrabbit-060725.txt, patch.txt, patch.txt, patch.txt, patch.txt > > > 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." -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://issues.apache.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira
