On 5/25/22 18:15, Van Haaren, Harry wrote: >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Wednesday, May 25, 2022 4:33 PM >> To: Eelco Chaudron <[email protected]>; Van Haaren, Harry >> <[email protected]> >> Cc: [email protected]; [email protected]; Stokes, Ian >> <[email protected]>; Amber, Kumar <[email protected]> >> 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. > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
