[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 5/7] migration: Deprecate old compression method
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v5 5/7] migration: Deprecate old compression method |
Date: |
Wed, 18 Oct 2023 09:13:58 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux) |
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.
[PATCH v5 6/7] [RFC] migration: Make -i/-b an error for hmp and qmp, Juan Quintela, 2023/10/17
[PATCH v5 4/7] migration: Deprecate block migration, Juan Quintela, 2023/10/17
[PATCH v5 7/7] [RFC] migration: Remove helpers needed for -i/-b migrate options, Juan Quintela, 2023/10/17
[PATCH v5 3/7] migration: migrate 'blk' command option is deprecated., Juan Quintela, 2023/10/17