Code review comment for lp:~hrvojem/percona-xtrabackup/docfix

On 16.01.2012 14:35, Alexey Kopytov wrote:
> Review: Needs Fixing
>
> Hrvoje,
>
> The changes look generally good to me. I just had a number of minor comments/questions:
>
> 1. |Percona Toolkit| is defined as a macro for ":term:`Percona Toolkit`", but is not actually used anywhere?
>
> There's also an inconsistency. |XtraBackup| and |Percona Server| are defined as just emphasized text rather than terms. Shouldn't |Percona Toolkit| be defined in the same way?
>
fixed
> 2. s/reffer/refer/g ?
fixed
>
> 3. Please keep "innobackupex" in examples and not replace it with "innobackupex-1.5.1", as the latter is an old name, it's currently just a symlink to innobackupex.
fixed
>
> 4. Any specific reasons for removing streaming backups from the replication setup guidelines? It's not like it's wrong, I'm just curious.
>
No special reason. I somehow figured out this was more simple solution.
But I can make another one with streaming or put it back as it was.

> 5. I'm not sure "mv /path/to/mysql/TIMESTAMP /path/to/mysql/datadir" will do the right thing. I think it will move all files to a subdirectory of datadir, which is not what we really want to do?

root@arrakis:/# mv /tmp/2012-01-16_11-14-43 /tmp/test/mysql
root@arrakis:/# ls -lah /tmp/test/mysql
total 31M
drwxr-xr-x 6 root root 4,0K Sij 16 11:19 .
drwxr-xr-x 3 root root 4,0K Sij 16 14:41 ..
-rw-r--r-- 1 root root 324 Sij 16 11:14 backup-my.cnf
drwxr-xr-x 2 root root 4,0K Sij 16 11:15 cacti
-rw-r--r-- 1 root root 18M Sij 16 11:19 ibdata1
-rw-r--r-- 1 root root 5,0M Sij 16 11:19 ib_logfile0
-rw-r--r-- 1 root root 5,0M Sij 16 11:19 ib_logfile1

>
> 6. Shouldn't we make it clear somehow that TIMESTAMP in examples should not be typed literally, but replaced with the actual directory name?

I've changed it to $TIMESTAMP and added the short note: You need to
select path where your snapshot has been taken, for example
/home/backups/2012-01-16_11-14-43

>
> 7. s/slights variation/slight variations/ ? (I know it's not your text, but might be a good idea to fix spelling once you are touching that line)
fixed
>
> 8. s/previuos/previous/
fixed
>
> 9. Please don't reuse branches for different MPs. As a result, this MP now contains fixes for both bug #878457 and bug #914422. Which makes it harder to manage and review.
yeah sorry about that one.