Re: [PATCH v5 5/7] migration: Deprecate old compression method

[Juan Quintela <quintela@redhat.com>]

Markus Armbruster <armbru@redhat.com> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
>> Acked-by: Peter Xu <peterx@redhat.com>


>>  # @deprecated: Member @disk is deprecated because block migration is.
>> +#     Member @compression is deprecated because it is unreliable and
>> +#     untested. It is recommended to use multifd migration, which
>> +#     offers an alternative compression implementation that is
>> +#     reliable and tested.
>
> Two spaces between sentences for consistency, please.

I have reviewed all the patches again.  Let's hope that I didn't miss
one.

>>  # @announce-step: Increase in delay (in milliseconds) between
>>  #     subsequent packets in the announcement (Since 4.0)
>>  #
>> -# @compress-level: compression level
>> +# @compress-level: compression level.
>>  #
>> -# @compress-threads: compression thread count
>> +# @compress-threads: compression thread count.
>>  #
>>  # @compress-wait-thread: Controls behavior when all compression
>>  #     threads are currently busy.  If true (default), wait for a free
>>  #     compression thread to become available; otherwise, send the page
>> -#     uncompressed.  (Since 3.1)
>> +#     uncompressed. (Since 3.1)
>>  #
>> -# @decompress-threads: decompression thread count
>> +# @decompress-threads: decompression thread count.
>>  #
>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>  #     bytes_xfer_period to trigger throttling.  It is expressed as
>
> Unrelated.

Rebases are bad for you O:-)

>> @@ -1023,7 +1036,9 @@
>>  # Features:
>>  #
>>  # @deprecated: Member @block-incremental is deprecated. Use
>> -#     blockdev-mirror with NBD instead.
>> +#     blockdev-mirror with NBD instead. Members @compress-level,
>> +#     @compress-threads, @decompress-threads and @compress-wait-thread
>> +#     are deprecated because @compression is deprecated.
>
> Two spaces between sentences for consistency, please.

Done.
>> @@ -1078,7 +1097,7 @@
>>  # Example:
>>  #
>>  # -> { "execute": "migrate-set-parameters" ,
>> -#      "arguments": { "compress-level": 1 } }
>> +#      "arguments": { "multifd-channels": 5 } }
>>  # <- { "return": {} }
>>  ##
>
> Thanks for taking care of updating the example!

You are welcome.  grep for all occurences of compress-level and friends
has its advantages.

>>  # @compress-wait-thread: Controls behavior when all compression
>>  #     threads are currently busy.  If true (default), wait for a free
>>  #     compression thread to become available; otherwise, send the page
>> -#     uncompressed.  (Since 3.1)
>> +#     uncompressed. (Since 3.1)
>>  #
>> -# @decompress-threads: decompression thread count
>> +# @decompress-threads: decompression thread count.
>>  #
>>  # @throttle-trigger-threshold: The ratio of bytes_dirty_period and
>>  #     bytes_xfer_period to trigger throttling.  It is expressed as
>
> Unrelated.

I have removed the periods.

But I have a question, why the descriptions that are less than one line
don't have period and the other have it.
>>      if (params->has_compress_level) {
>> +        warn_report("Old compression is deprecated. "
>> +                    "Use multifd compression methods instead.");
>>          s->parameters.compress_level = params->compress_level;
>>      }
>>  
>>      if (params->has_compress_threads) {
>> +        warn_report("Old compression is deprecated. "
>> +                    "Use multifd compression methods instead.");
>>          s->parameters.compress_threads = params->compress_threads;
>>      }
>>  
>>      if (params->has_compress_wait_thread) {
>> +        warn_report("Old compression is deprecated. "
>> +                    "Use multifd compression methods instead.");
>>          s->parameters.compress_wait_thread = params->compress_wait_thread;
>>      }
>>  
>>      if (params->has_decompress_threads) {
>> +        warn_report("Old compression is deprecated. "

Once here, I did s/Old/old/

as all your examples of description start with lowercase.

>> +                    "Use multifd compression methods instead.");
>>          s->parameters.decompress_threads = params->decompress_threads;
>>      }
>
> Other than that
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks for your patience.