[
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
