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

[Markus Armbruster <armbru@redhat.com>]

Juan Quintela <quintela@redhat.com> writes:

> 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.

When the description consists of multiple sentences, we obviously need
to end them with punctuation.

Sometimes the description isn't a sentence, just a phrase,
e.g. "decompression thread count".  No punctuation then.

Sometimes it's a single sentence.  Then we roll dice.

>>>      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.

Yes, please.

>>> +                    "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.