Hema,
I am using a new review format I have proposed to Lars. Let me know what
you thing. By way of terse introduction, there are these sections:
STATUS - approved/not approved/approved pending changes
REQUIRED - things you must fix
REQUESTS - things you must either fix or explain why you will not fix
SUGGESTIONS - optional changes that you are strongly encouraged to fix
COMMENTARY - critique/commiseration
DETAILS - abbreviated annotation to support above
Chuck
=========================================================================
STATUS
------
Approved pending changes
REQUIRED
--------
1. Fix patch comments.
2. Fix comments in test -- many are wrong.
3. Where does c) and d) occur in the test? There are no comments (in the
test body) to indicate where this is occurring. You did so for a) and
b). Also, I would make it more clear by copying the statements from the
test block header.
REQUESTS
--------
4. Please change comments from ### to --echo #.
SUGGESTIONS
-----------
5. Change comments in test header.
6. Make more comments visible in output.
COMMENTARY
----------
I realize the requested change above was in the original test so this is
a request for refactoring -- and a simple one at that.
DETAILS
-------
> WL#4232(Test native drivers). This worklog has 2 tests included in the backup
> suite
> 1. backup_vp_nontx_memory.test
> 2. backup_vp_nontx_myisam.test
[1] This is not enough information. As Ingo points out so well, the
patch comments should tell us a) what is the problem, and b) what this
patch does to fix the problem.
...
> === added file 'mysql-test/suite/backup/include/backup_vp_nontx.inc'
> --- a/mysql-test/suite/backup/include/backup_vp_nontx.inc 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/backup/include/backup_vp_nontx.inc 2009-05-20 21:11:22 +0000
> @@ -0,0 +1,262 @@
> +#
> +# This test is one of two validity point tests. See:
> +# backup_vp_tx.test
[5] I don't like this wording. Please remove this statement or make it a
footnote. The first lines in the test should explain what this test is,
why it was created, and how it works. The information below is correct.
Adding this line as the first in the header block is confusing and
irrelevant. It is, however, worthy of a footnote.
> +#
> +# The goal of the test should be to ensure the following assumptions
> +# for the behavior of validity point (VP) hold. Validity point is
> +# implemented using commit blocker (CB).
> +#
> +# Note: in this file, "DML" refers to DML operations executed in
> +# non-transactional storage engines.
> +#
> +# a) DMLs that are executed before VP are in the backup image
> +#
> +# b) setting the validity point should not be done while DMLs are
> +# being processed in any table involved in the backup. An active
> +# DML therefore blocks backup from setting commit blocker. The DML
> +# has to complete before backup can set CB (and ultimately set the
> +# VP), and will therefore be in the backup image
> +#
> +# c) setting the validity point should not be done while DMLs are
> +# being processed in any table involved in the backup. A DML
> +# operation requested when backup is ready to set VP is therefore
> +# blocked by CB. The DML has to wait for CB to be released before
> +# it can continue, and will therefore not be in the backup image
> +#
> +# d) DMLs executed after VP are not in the backup image
> +#
> +#
> +
...
> +
> +--echo
> +--echo con_bup: Show the data before we start backup
> +SELECT * FROM bup_vp.t1 ORDER BY col_a;
> +
> +### CON 1 ###
> + --echo
[2] This and many other comments indicate the wrong connection. Please
fix these in all tests in this patch.
[4] Please change the above comment to the following:
--echo #
--echo # Connection con_ntx1
--echo #
Rationale: Well, first, the comment is wrong. ;) Second, I like this new
style that Ingo and others have been requesting for test comments. It
serves two purposes 1) it shows up in the result file, and 2) it is
uniform. Let's switch to this style for interdelinia comments. The big
comment block at the top is still ok -- no need to change that.
> + connection con_ntx1;
> +
> + --echo Scenario (b): Insert in progress blocks CB and is included in backup
> + --echo con_ntx1: Start insert and stop it in the middle of processing
> +
> + --echo con_ntx1: Make insert stop in the middle of execution
> + # Will continue once backup has been blocked from setting CB
> + SET DEBUG_SYNC= 'after_insert_locked_tables SIGNAL insert_started
> + WAIT_FOR complete_insert';
> + send INSERT INTO bup_vp.t1 VALUES ("ntx1: 06 Some data to test");
> +
> +
> +### CON BUP ###
> +--echo
[2] Again here...I will cease identifying the rest -- there are many
such occurrences.
> +connection con_bup;
> +
>
> +# Backup will be blocked from setting CB by the ongoing insert in
> +# con_ntx1. Backup will signal con_ntx1 that it has been blocked. The
> +# insert will then finish, making backup able to set CB. When CB has
> +# been set, ntx2 will be signaled to try to insert. ntx2 insert will
> +# be blocked by the CB and send a signal that it has been blocked. CB
> +# is then released, but backup is stopped immediately after releasing
> +# CB. ntx2 will now be able to complete the insert, and ntx1 will
> +# issue and complete another insert. Finally, backup is allowed to
> +# complete.
> +
> +--echo con_bup: Activate synchronization points for BACKUP.
> +# Signal when backup is blocked by ongoing insert in con_ntx1
> +SET DEBUG_SYNC= 'wait_lock_global_read_lock SIGNAL sync_complete_insert';
> +
> +# ntx1 insert is completed, so backup can set CB. Just before reaching
> +# VP, signal ntx2 should try to insert. Wait until ntx2 signals it has
> +# been blocked
[6] Some of these statements should be in --echo # comments.
> +SET DEBUG_SYNC= 'before_backup_data_lock SIGNAL try_insert
> + WAIT_FOR insert_blocked';
> +
> +# ntx2 insert has been blocked. Create VP and release CB, and then wait
> +# while ntx2 and ntx1 inserts.
[6] again...
> +SET DEBUG_SYNC= 'after_backup_binlog SIGNAL commit_unblocked
> + WAIT_FOR finish_bup';
> +
...
EOF
