Hi Øystein,
Thanks for reviewing the online backup patches, my comments are
in-line for the issues , i did not respond in my earlier e-mail.
Øystein Grøvlen (JIRA) wrote:
[ http://issues.apache.org/jira/browse/DERBY-239?page=comments#action_12361535 ]
Øystein Grøvlen commented on DERBY-239:
---------------------------------------
I have reviewed the latest patches (3 through6). (A bit late I must admit).
Here are my comments:
java/engine/org/apache/derby/catalog/SystemProcedures.java
* SYSCS_BACKUP_DATABASE()
- "By default this procedure will wait ..." Is it possible to
change this behavior for this particular procedure? If not,
"by default" is a bit misleading
- "wait for the backup blocking unlogged operations to complete
..." is a bit heavy. I suggest just "wait for any unlogged
operations to complete ..."
I will fix the comments.
* SYSCS_ONLINE_BACKUP_DATABASE_AND_ENABLE_LOG_ARCHIVE_MODE()
- Same comment on ONLINE as above
- Could not boolean parameters be used now?
I am not sure boolean paramets work with stored procedures now.
Even if the support available, just to be consistent with existing
backup procedure parameters types, I think using INT is better for the
new procedures also.
* backupDatabaseAndEnableLogArchiveMode()
- Most of the code here is the same as in backupDatabase().
(Another argument for pushing this code down to a lower layer.)
To avoid code duplication, whether to enable archiving could have
been a flag to backupDatabase.
Added this function to support implicit commit/rollback, This function
may not be needed any more. I will just call the low level backup
calls directly.
* SYSCS_DISABLE_LOG_ARCHIVE_MODE()
- Is checkBackupTransactionIsIdle() strictly necessary here? This
seems like an operation where failures could be handle at
statement level.
No. Idle check was necessary for issuing implicit commit to be
consistent with other backup calls.
java/engine/org/apache/derby/iapi/store/access/AccessFactory.java
* backup(String ...)
- "Please see cloudscape on line ..." Derby?
* backup(File ...)
- I will just remind you of DERBY-665. I could do that work, but
I am not sure it is a good idea for other people change the
backup code while you are working on it. (May create merge
conflicts for you.)
Thanks for reminding , I will fix it.
java/engine/org/apache/derby/iapi/store/raw/xact/RawTransaction.java
* setBackupBlockingState()
- I do not like the name for this method. I suggest calling it
blockBackup() or something like that. At least, the javadoc
should explain what is meant by "backup blocking state".
Thanks for the suggestion , I will change the names to blockBackup()
and unblockBackup() .
java/engine/org/apache/derby/iapi/store/raw/xact/TransactionFactory.java
* stopBackupBlockingOperations()
- Name indicates that backup blocking operations are stopped, but
javadoc says that only new ones are blocked. I think the name
is misleading.
- Javadoc should be revisited for typos.
I can change them too :
blockBackupBlockingOperations()
unblockBackupBlockingOoperation()
Any suggestions ?
java/engine/org/apache/derby/impl/db/BasicDatabase.java
* backupAndEnableLogArchiveMode()
- Non-standard indentation for parameters?
Not sure why I did that, will fix it.
java/engine/org/apache/derby/impl/store/raw/RawStore.java
* backup(String, boolean)
- Typo in comment: "Check if there any backup ..." Remove "there"?
* backupAndEnableLogArchiveMode(String, boolean)
- Why do you need a finally clause? Would not a catch clause be
sufficient? Then, you could eliminate the local 'error'
variable.
if I catch on Throwable , may be I don't need finally in this case.
java/engine/org/apache/derby/impl/store/raw/data/RFResource.java
* add()/remove()
- Are the casts to RawTransaction safe? Does this assumption have
any impact on the modularity of the code?
Yes. RawTransaction is the one that implements Transaction, and the
way the Transaction is acquired it is sure to be RawTransaction.
Having said that I don't like the casting either , will look around
and see if there is a simple way to get RawTransaction directly.
* serviceImmediately()
- How is this change related to backup?
If the jar files are not removed on COMMIT , removal work gets
assigned to RawStoreDaemon(postcommit) thread. Backup of jars and
Removal of Jar can conflict. Blocking the RawStoreDaemon on backup may
not be a good idea , so change the removal of jars to happen on the
commit calls itself and also I think it better remove files on commit.
java/engine/org/apache/derby/impl/store/raw/xact/Xact.java
* setBackupBlockingState()/setUnblockBackupState()
- Names should be symmetric. (e.g., blockBackup/unblockBackup)
- Why do you have to wait for commit to unblock? Would it not be
sufficient to have completed the unlogged operations before
backup is started?
May be backup can be allowed once the unlogged operation is complete,
I have not looked in-depth to allow immediately after the unlog
operations is complete, one case that I thought likely to have
problem is JAR file removal on post commit conflicting with backup
jar copy operation.
For now, I think enforcing the COMMIT restriction is ok.
java/engine/org/apache/derby/impl/store/raw/xact/XactFactory.java
* canStartBackupBlockingOperation()
- Since this method now may wait for backup to complete, I do not
feel canStartBackupBlockingOperation() is a good name.
- Symmetric naming with backupBlockingOperationFinished() would
make it more evident that these two functions should be called
in pairs.
will change these name also too :
blockBackup() and unblockBackup()
java/engine/org/apache/derby/loc/messages_en.properties
* XSRSA.S
- Suggest the following change:
"Cannot backup the database when unlogged operations are uncommitted. Please commit the transactions with backup blocking operations or use the backup procedure with option to wait for them to complete."
- Generally, I am not very fond of these long error messages. I
think it would be better with just a single sentence, and then
the user should be able to look up an explanation in a manual.
* XSRSB.S
- Suggest the following change:
"Backup operation can not be performed in an active transaction. Please start a new transaction to execute backup procedures."
Thanks for the suggestion , I will update the error messages.
java/testing/org/apache/derbyTesting/functionTests/tests/store/OnlineBackupTest1.java
* runTest()
- Should have more than one unlogged operation in parallel in
order to fully test that the counting of unlogged operations
work.
- Suggest to add test that when doing backup in a non-idle
transaction, previous work has not been rolled back when backup
fails, and that one can continue with more operations within the
same transaction.
* runConsistencyChecker()
- This does only check consistnecy of internal structures. Should
also check consistency of application data. Maybe you could
execute a select?
* performDmlActions()
- I assume the intention here is to do "while (!stopActivity)"
I will enhance the test to address the above mentioned cases.
* endUnloggedAction()
- What is the role of the insert? Is is unlogged? It is not
evident from the method name or Javadoc why you are doing
inserts here.
I intended to check if that insert will make it to backup.
* select()
- What is the point of doing consistency checks on columns that
are not updated? If id and name does not match, that must be
caused by errors in code that is not particular to backup.
u are right, I will change the select to check on columns that are
being updated.
java/testing/org/apache/derbyTesting/functionTests/tests/store/OnlineBackupTest3.java
* installJarTest()
- Typos in comment: "// followng backup call should because jar
operation is pending". Should what?
- Comment say: "//Now commit the jar operation in connection1 for
backup to proceed." The next statement does an insert. This is
confusing.
- It would be nice if the test checked that jar operation is still
waiting for backup when create index has completed, but I guess
this is a bit difficult to achieve.
* A mix of tab and spaces for indentation. For new files that
should not be necessary!
* removeJarTest()
- Comment copied from installJarTest?
"// wait for customer app jar installation to finish now. "
I will fix them.
java/testing/org/apache/derbyTesting/functionTests/tests/store/onlineBackupTest2.sql
* Do you have idea of how frequently it happens that backup thread
has not been blocked yet when backupdir is created?
In this test,If backup is blocking correctly on unlogged operations
backupdir should not get created.
How long a
sleep would you need to be certain?
I checked with 1000 msec sleep time, test failed on my laptop if
backup is not blocking correctly for the unlogged operations to commit.
Test will not fail even if sleep() is removed , it is just to make
sure there is a chance for the test to actually enter the backup
blocking code. I can not be certain how much sleep time will make
sure that will happen, it will depend on the hardware and OS on which
the test is being run.
Thanks
-suresht
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: obtest_customer.jar, onlinebackup.html, onlinebackup1.html,
onlinebackup_1.diff, onlinebackup_2.diff, onlinebackup_3.diff,
onlinebackup_4.diff, onlinebackup_5.diff, onlinebackup_6.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.
