[
http://issues.apache.org/jira/browse/DERBY-239?page=comments#action_12355620 ]
Øystein Grøvlen commented on DERBY-239:
---------------------------------------
I have looked at the onlinebackup_1.diff patch, and overall the patch
looks good. However, I think there is one severe bug in
RAFContainer.java. I guess due the cut and paste from writePage(),
you are seeking in another file than the one you are writing
to (fileData.seek(), but backupRaf.write()).
I have a few other comments and questions:
General:
* If I run backup with this patch, it seems like I will run the
new code. Does one not need to change the restore code to be
able to restore restore such a backup, or does that the
ordinary recovery handle that?
RawStore.java:
* The BACKUP_FILTER now contains so much, that it would be useful
to have a comment that says what is really left to copy.
* Intuitively, it seems wrong to hard-code "seg0", but I see that
this is done all over the code. Will there always be only one
segment? What is then the purpose of the segment concept?
* backup(File ...) seems like it would create an endless recursion
if called. Fortunately, it seems like it never will be
called. Why do we need the methods with a File parameter instead
of a String. The system procedures uses the String variant.
Maybe we could just remove the File variant?
* I think it would be helpful with a comment that explained why
disableLogArchiveMode() was made synchronized.
BaseDataFileFactory.java:
* I do think basing which files to back up on the contents of the
seg0 directory is very robust. What if someone by accident has
written a file with a name that matches the pattern you are
looking for. Then I would think you may get a very strange
error message that may not be easy to resolve. Could not this
be based on some system catalog? Another scenario is if someone
by accident deletes a file for a table that is not accessed very
often. A later backup will then not detect that this file is
missing. Since the backup is believed to be succesful, the
latest backup of this file may be deleted.
FileContainer.java:
* I cannot find any backup-specific about getPageForBackup() so I
think a more general name would be better (e.g, getLatchedPage).
RAFContainer.java:
* The changes to this file seems not to be quite in line with some
of the original design philosophies. I am not sure that is
necessarily bad, but it would be nice to here the arguments for
doing it this way. More specifically:
- While RAFContainer so far has used the
StorageRandomAccessFile/StorageFile abstractions, backup
use RandomAccessFile/File directly. Is there a particular
reason for that?
- In order to be able to backup a page, new methods have
been added to FileContainer and BasePage/CachedPage, and
the RAFContainer is doing latching of pages. This
increases coupling to other modules, have alternative
designs been considered?
* privBackupContainer():
- Setting 'done=true' at the start of the method is a bit
confusing. I would think another name for this variable
would be better.
- If an exception is thrown while holding a latch, do one
not need to relase the latch?
* writeToBackup():
- copies a lot of code from writePage(). One should
consider factoring out common code.
- Due the cut and paste, you are seeking in another file
than the one you are writing to. (fileData.seek(), but
backupRaf.write())
LogToFile.java:
* In getFirstLogNeeded why did you need to change
getFirstLogNeeded() to handle a null checkpoint? Is it in case
you do backup before any checkpoint has been performed?
* The javadoc for startLogBackup() says that 'log files are copied
after all the data files are backed up, but at the end of the
method log files are copied.
ReadOnly.java/InputStreamContainer.java:
* I do not think javadoc should just be a copy of the
interface/superclass, but say something about what is particular
to this implementation. In this case, the Javadoc should say
that nothing is done.
typos:
* several occurences of 'backedup'
* LogToFile.java: 'eventhough'
* RAFContainer.java: 'pahe cache', 'conatiner'
* 'Standard Cloudscape error policy'. Should this be changed to
Derby?
Pet peeves:
* Should have Javadoc on all methods, parameters and return
values.
* Lines longer than 80 chars.
> Need a online backup feature that does not block update operations when
> online backup is in progress.
> --------------------------------------------------------------------------------------------------------
>
> Key: DERBY-239
> URL: http://issues.apache.org/jira/browse/DERBY-239
> Project: Derby
> Type: New Feature
> Components: Store
> Versions: 10.1.1.0
> Reporter: Suresh Thalamati
> Assignee: Suresh Thalamati
> Attachments: onlinebackup.html, onlinebackup_1.diff
>
> Currently Derby allows users to perfoms online backups using
> SYSCS_UTIL.SYSCS_BACKUP_DATABASE() procedure, but while the backup is in
> progress, update operations are temporarily blocked, but read operations can
> still proceed.
> Blocking update operations can be real issue specifically in client server
> environments, because user requests will be blocked for a long time if a
> backup is in the progress on the server.
--
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
