[ 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 ..." * SYSCS_ONLINE_BACKUP_DATABASE() - Since both backup procedures are ONLINE, it is a bit misleading to use this word to distinguish between the two backup procedures. I guess the main reason for choosing a new name is the extra parameter. In that case, I think it would be better to name the new procedure, SYSCS_BACKUP_DATABASE_NOWAIT and leave out the parameter. - The javadoc does not say what will happen if wait is 0. Will one get an exception if there is unlogged operations? * backupDatabase() - Is this the right layer for checking that the transaction is idle and for doing rollback/commit the transaction? Since this is a requirement for the logic at lower layers to work correctly, not something that is done because it is the desirable behavior of the system procedure, I feel that this should be done at a lower layer. - I know when we discussed this isssue earlier, I agreed that checking that the transaction is idle was a good solution. However, thinking a bit more about this, I think it would be better to fail the transaction when unlogged operations have been performed by the same transaction. That would limit it to those who actually need to be affected, and it would significantly reduce the probability of someone ever experiencing this problem. - I am not very fond of automatic commits like this. If this is necessary, I think it would be better to require that backup is performed in autocommit mode. Then the implications would be more evident to the users and not catch someone by surprise. - The javadoc for the system procedures that use this function should state the requirement imposed here (idle transaction) and that the transaction will be committed if backup is succesful. * SYSCS_ONLINE_BACKUP_DATABASE_AND_ENABLE_LOG_ARCHIVE_MODE() - Same comment on ONLINE as above - Could not boolean parameters be used now? * 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. * SYSCS_DISABLE_LOG_ARCHIVE_MODE() - Is checkBackupTransactionIsIdle() strictly necessary here? This seems like an operation where failures could be handle at statement level. 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.) 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". 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. java/engine/org/apache/derby/impl/db/BasicDatabase.java * backupAndEnableLogArchiveMode() - Non-standard indentation for parameters? 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. 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? * serviceImmediately() - How is this change related to backup? 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? 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. 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." 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)" * 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. * 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. 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. " 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? How long a sleep would you need to be certain? > 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. -- 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