Mike Matrigali wrote:
I have tested this patch and reviewed the changes and
have commited it as svn 345355.
I have the following
suggestions from review, and will look at any subsequent
changes in these areas that Suresh would like to submit:
o RawStore.java!canStartOnlineBackup() has a todo item - needs an
exception error to be thrown.
Working on this one, tests needed some changes when exception is
thrown. Deferred it to fix it along with tests changes for the fix to
do commit/rollback after the backup procedures.
o will things like sort and temporary containers cause online
backup to wait if they are outstanding?
Backup should wait only for the real containers with non-logged operation.
Does the pre-existing
backup cause sorts and temp tables to
be logged during online backup?
(these questions probably apply to BaseDataFileFactory.java)
Temp tables are not logged during the backup. I thought sort uses temp
tables, will check on this one.
Please let me know if you notice any of my changes are not doing the
above.
o could you pick one place in the code to describe how all the
routines work together to provide the functionality you need.
I think basically put the description that you have in the
JIRA for this patch somewhere in the code. The individual routines
have comments but it hard to see how they all work together
without one place descibing the interaction.
sure. I can do that in the main backup() routine in RawStore.java.
In future may be backup code in RawStore.java should moved into a
different BackupFactory class.
o as I was reviewing I fixed some >80 line stuff.
I know sometimes it is hard, but
at least stuff like comments is really easy.
hmm.. Thanks for fixing them. I better add some check to my emacs to
avoid this in future.
o may be interesting to add external sort testing and queries
that cause temp tables. probably dependent on answer to above
question.
I will add a test case to make sure backup is not blocked on temp tables.
Thanks
-suresh
