>>>>> "ST" == Suresh Thalamati <[EMAIL PROTECTED]> writes:
ST> Hi Ãystein,
ST> Thanks for reviewing the patch. My answers are in-line...
ST> Ãystein Grøvlen (JIRA) wrote:
...
>> * Intuitively, it seems wrong to hard-code "seg0", but I see that
>> this is done all over the code.
ST> Could you please file a JIRA entry for this one. I noticed it too,
ST> hard coding is definitely not the correct way here. One of us can
ST> clean up this.
Done. Derby-664.
...
>> * 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?
ST> I think they are left overs from the old cloudscape versions, File
ST> variant is not supported any more , these can be cleaned up. There
ST> should be some File variant calls in access layer also, Could you
ST> please file Jira for this cleanup, so that these won't forgotten
ST> again.
Done. Derby-665.
...
>> 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?
ST> I think users should not be creating/deleting any files in SEG0 to
ST> start with. Pattern matching is just a simple precaution. I thought of
ST> scanning the catalogs to find the container to backup, but could not
ST> convince my self that it is required at least for now.
I agree that users should not mess with seg0. However, I think part
of being an easy-to-use database, is to be able to give meaningful
error messages when users mess things up.
ST> If you think scanning the catalogs is more robust, this enhancement
ST> can be done at later point of time easily.
Ok. I will see if I feel the itch. 8-)
>> 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.
>>
ST> Backup is as good as the database in this case :-) If users access the
ST> deleted file table from the backup DB , it will fail with NO container
ST> error, as it will on the main database.
ST> By using the catalog approach also backup can only throw a warning
ST> about the deleted container. Once the user deletes a container file ,
ST> there is no way out, users can not even drop that table. I don't think
ST> making backup fail forever when this scenario occurs will be
ST> acceptable to the users.
I agree, but detecting in on backup, gives the user the option of
recovering the table by restoring the previous bakcup.
>> FileContainer.java:
>> * I cannot find any backup-specific about getPageForBackup() so I
>> think a more general name would be better (e.g, getLatchedPage).
>>
ST> Yes, currently this routine does not have any backup specific stuff in
ST> this routine. I would like to get the page from the cache with
ST> specific weight for the backup page in future, when the weight based
ST> cache mechanism is implemented.
In that case, I would suggest that the weight be a parameter to the
method, not part of the name.
>> 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?
ST> Yes. Currently there is No API available for the users to specify the
ST> type of Storage Factory to use for the backup. It will be good
ST> enhancement to add later.
ST> Online backup implementation is doing what the current backup does;
ST> i.e use the disk IO calls directly , instead of the database Storage
ST> Factory. I think one reason to do this way is if a database is using
ST> in-memory Storage Factory, users can backup the database on to a
ST> disk. If Storage Factory used by the Database is java.io.* , then
ST> backup is doing the same without going through the Storage Factory
ST> interface.
Makes sense. If one want a StorageFactory for backup, one can add that
later.
>> - 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?
ST> I was considering doing following minor changes here:
ST> 1) Instead of getting pageData byte array from the Page Object, make
ST> a request to the page to write it self to backup(like page.backup(....).
Sounds good to me. Then much the same structure is used for backup as
for clean.
ST> 2) Move the backing up and unlatching of the page code to the
ST> FileContainer.java.
ST> I have not thought of any other better ways to do design this, any
ST> ideas will be helpful.
Could the backup code be in a separate class instead of part of
RAFContainer? It does not seem to use much of RAFContainer's internal
state.
...
--
Øystein
