On 5/27/22 14:09, Ilya Maximets wrote:
> On 5/25/22 18:15, Van Haaren, Harry wrote:
>>> -----Original Message-----
>>> From: Ilya Maximets <i.maxim...@ovn.org>
>>> Sent: Wednesday, May 25, 2022 4:33 PM
>>> To: Eelco Chaudron <echau...@redhat.com>; Van Haaren, Harry
>>> <harry.van.haa...@intel.com>
>>> Cc: i.maxim...@ovn.org; ovs-dev@openvswitch.org; Stokes, Ian
>>> <ian.sto...@intel.com>; Amber, Kumar <kumar.am...@intel.com>
>>> Subject: Re: [PATCH] dpcls: revert subtable-lookup-prio-get name change
>>>
>>> On 5/25/22 16:32, Eelco Chaudron wrote:
>>>>
>>>>
>>>> On 25 May 2022, at 16:10, Harry van Haaren wrote:
>>>>
>>>>> This commit reverts the name-change that was done (prio->info).
>>>>> The change breaks a user visible ovs-appctl command, resulting in
>>>>> breakage of tools/scripts/user-expectation outside of the OVS repo.
>>>>>
>>>>> This commit changes the documentation, command string, and unit tests
>>>>> back to the expected "prio" string, as expected in OVS 2.17 and earlier.
>>>>
>>>> Can’t remember all the details, but I guess Ilya’s concern was that the
>>>> command
>>> name does not reflect the actual output.
>>>>
>>>> So what about hiding the “subtable-lookup-prio-get” command, and making it
>>>> such
>>> that it still works for backward compatibility?
>>>> I think this can easily be done by adding the command with a NULL usage
>>>> string.
>>>>
>>>> I guess all that needs to be added is another command, something like, but
>>>> please
>>> test it:
>>>>
>>>> + unixctl_command_register("dpif-netdev/subtable-lookup-prio-get",
>>>> NULL,
>>>> + 0, 0, dpif_netdev_subtable_lookup_get,
>>>> + NULL);
>>>>
>>>> Harry/Ilya what do you think?
>>>
>>> The unlisted alias looks like a good middle ground to me.
>>
>> "subtable-lookup-prio-get" lists the various implementations, their
>> use-count, and priority. Sample output:
>> # ./ovs-appctl dpif-netdev/subtable-lookup-prio-get
>> Available dpcls implementations:
>> autovalidator (Use count: 0, Priority: 0)
>> generic (Use count: 0, Priority: 1)
>> avx512_gather (Use count: 2, Priority: 3)
>>
>> That's a pretty good name for a function that gets the priority of the
>> subtable lookup functions.
>
> I know how the output looks like. And, as you said yourself,
> this command now lists the implementations, hteir use-count
> and priority. So, the priority is the *third* thing that it
> lists. The purpose of that command changed, hence the name.
>
>> It exists today, users and scripts already use it. Unfortunately its name
>> was changed, and that
>> broke user scripts and command histories. This patch fixes it to be
>> consistent again.
>
> What's wrong with an alias? It preserves the backward compatibility,
> so there should not be any problems.
>
>>
>>
>>> The original patch is also missing the NEWS file update that
>>> should be added, mentioning the re-name.
>>>
>>> At the same time, I'm not sure if we actually need to maintain
>>> backward compatibility here as this command can hardly be used
>>> in automated scripts. The output format is changed significantly,
>>> so any automated tools will have to be modified anyway.
>>
>> Consider a script that sets the autovalidator, runs for 5 seconds, and then
>> turns on the scalar again.
>> It does not need to parse the output, so if we leave the command as before,
>> it will continue to work.
>> If existing scripts were sed/grep/awk parsing out the "priority: value", it
>> will continue to work.
For the record, that is not true. The old format was
'<value>: name', so any sed/grep/awk will fail to parse
the new format, which is 'Priority: <value>'.
>> If we change the name, we've just broken our users scripts.
>
> Sure, but we're not changing the name of the 'set' command.
>
>>
>>
>>> In general, appctl APIs are supposed to be used mostly by humans,
>>> especially "get/show/etc" ones as we're not maintaining the exact
>>> output format for them. And if we're not maintaining the output
>>> format, there is no much value in maintaining the command name.
>>
>> There is value in maintaining command-name stability -> see above example.
>> It shouldn't matter if things are supposed to be used specific ways in
>> general,
>> reality is that these commands are put into scripts, and renaming it will
>> break.
>>
>> I don't know what else to say, renaming/breaking commands has no value in my
>> eyes, and only
>> makes using the command difficult.
>>
>> This patch should be merged ASAP, to return the command to the known command
>> string,
>> and fix the current breakage of the user's scripts/command-history.
>
> Again, what's wrong with the backward compatible alias?
>
>>
>>
>>> Best regards, Ilya Maximets.
>>
>> Regards, -Harry
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
