mylvmbackup-discuss team mailing list archive

[Lenz Grimmer <lenz@xxxxxxxxxxx>]

Hi Linda,

again thank you for your contribution and sorry for the delay in responding.

On 09/15/2015 12:12 AM, Linda Xu wrote:

> I uploaded the changes to launchpad as
> "lp:~lxu8/mylvmbackup/mylvmbackup-0.16lx
> <https://code.launchpad.net/~lxu8/mylvmbackup/mylvmbackup-0.16lx>"
>
> Only the main script mylvmbackup is been changed. Please let me know if
> there is any question or other modification needed.

I finally found some time to review your changes. Unfortunately reviewing and
merging your changes is somewhat difficult, as the bzr tree does not seem to
be a full clone of the "trunk", but rather a single checkin of an extracted
source tarbell.

In order to preserve the commit history, and to ease the process of merging
your changes, would it be possible for you to amend your contribution as follows?

Please create a full branch off the trunk by using "bzr branch
lp:mylvmbackup". Then, apply your modifications on top of this tree, in file
mylvmbackup.pl.in (not the final mylvmbackup file).

Commit and push your modifications to a new branch on Launchpad, e.g. "bzr
push lp:~lxu8/mylvmbackup/mylvmbackup-ftwr". This would allow me to cleanly
merge your changes and maintain your commit messages.

The comments you added below the GPL header look somewhat truncated, some
sentences seem to be incomplete and end with "»" instead. So it is difficult
to fully understand the various modes.

A question about the $stop_replication option: enabling this option issues a
"STOP SLAVE SQL_THREAD" before the tables are locked. How would this affect
temporary tables? According to the documentation, there are some things you
have to consider in this case:

https://dev.mysql.com/doc/refman/5.6/en/replication-features-temptables.html

I also have a pending bug report about temporary tables, so I wonder if this
is related: https://bugs.launchpad.net/mylvmbackup/+bug/1217747

Can you please comment on this?

With regards to the new options you want to introduce: I find "flush_ftwrl_*"
somewhat long and redundant. How about removing the "flush_" prefix?

Instead of describing the options in the code, would you mind adding these,
including your complete comments to the mylvmbackup.conf file?

Some general comments: would you mind to adjust your indendations so they
match the rest of the code, which uses two spaces per indendation level?

Also another style question, could you please put SQL keywords in uppercase,
e.g. the ones you defined in $find_long_query_cnt and so on.

Thank you very much!

Lenz

-- 
  Lenz Grimmer <lenz@xxxxxxxxxxx> - http://www.lenzg.net/

Attachment: signature.asc
Description: OpenPGP digital signature