Code review comment for lp:~akopytov/percona-xtrabackup/bug1095249-2.1

Hi George,

On Thu, 03 Jan 2013 17:47:22 -0000, George Ormond Lorch III wrote:
> Review: Approve g2
>
> Looks good except for tw things:
> 1 - diff line 292, #include "ds_buffer.h" in ds_compress.c, is that really necessary since in 2.1 ds_compress has no explicit interaction with ds_buffer?
> 2 - there is no call to ds_buffer_set_size to set the buffering size to 1M from the default 64K...does it make sense to make this a program option?
>

Good catch. Removed redundant includes and added a ds_buffer_set_size()
call.

Regarding making it a program option. We have
https://blueprints.launchpad.net/percona-xtrabackup/+spec/specify-io-block-size
which can make both read and write buffers configurable. So far they are
both hardcoded to 1 MB.

> This brings up two issue in/with the 2.1 encryption work, small writes from compress->encrypt (encrypting extremely small buffers) and inefficient i/o. Also IIRC the encryption work implemented similar test case for basic encryption as I found it was missing. Should probably echo these comments on that MP and fix topology creation and destruction accordingly. This could be our first case of using multiple instances of a single data sink if we choose to go that route:
> ds_compress -> ds_buffer -> ds_encrypt -> ds_buffer -> ds_local
>

Yes, I see encryption also writes metadata in a separate write call, so
it will still be affected by the same problem even when compression will
use buffering (or no compression is used).

Thanks!