Suresh,
Just a reminder of some things you promised to fix in the previous
review round.
--
Øystein
>>>>> "ST" == Suresh Thalamati <[EMAIL PROTECTED]> writes:
...
>> 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.
ST> I agree, comments will be helpful here. I am not sure BACKUP_FILTER
ST> will be be needed any more. Currently it just does not filter JAR dir,
ST> I plan to make JARS also as separate copy after the data segment as
ST> discussed in my previous e-mail.
...
>> * I think it would be helpful with a comment that explained why
>> disableLogArchiveMode() was made synchronized.
ST> Yes, I definitely need to add comments about this synchronization.
ST> This was part of my attempt to prevent parallel backup actions, which
ST> I did not complete as part of this patch.
...
>> RAFContainer.java:
...
>> * 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.
ST> I will change it to 'backupComplete = false' and reverse the exit
ST> logic, hopefully that will make it more readable.
>> - If an exception is thrown while holding a latch, do one
>> not need to relase the latch?
ST> Yes. Latch has to be released. I will make sure no latches are held
ST> when an error occurs
>> * writeToBackup(): - copies a lot of code from writePage(). One
>> should
>> consider factoring out common code.
ST> I will move the common code into a separate routine.
...
>> LogToFile.java:
...
>> * 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?
ST> I will correct the comments.
>> Pet peeves:
>> * Should have Javadoc on all methods, parameters and return
>> values.
>> * Lines longer than 80 chars.
>>
ST> Sound like a good ones to follow. I will add the java doc and fix any
ST> long lines.
ST> Thanks
ST> -suresh
