Re: [PATCH] migration: Allow user to specify migration available bandwidth

[Peter Xu <peterx@redhat.com>]

On Fri, Aug 04, 2023 at 02:39:15PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jul 26, 2023 at 11:12:31AM -0400, Peter Xu wrote:
> > On Wed, Jul 26, 2023 at 08:21:35AM +0200, Markus Armbruster wrote:
> > > Peter Xu <peterx@redhat.com> writes:
> > > 
> > > > Hi, Markus,
> > > >
> > > > On Tue, Jul 25, 2023 at 01:10:01PM +0200, Markus Armbruster wrote:
> > > 
> > > [...]
> > > 
> > > >> For better or worse, we duplicate full documentation between
> > > >> MigrationParameter, MigrateSetParameters, and MigrationParameters.  This
> > > >> would be the first instance where we reference instead.  I'm not opposed
> > > >> to use references, but if we do, I want them used consistently.
> > > >
> > > > We discussed this over the other "switchover" parameter, but that patchset
> > > > just stranded..
> > > >
> > > > Perhaps I just provide a pre-requisite patch to remove all the comments in
> > > > MigrateSetParameters and MigrationParameters, letting them all point to
> > > > MigrationParameter?
> > > 
> > > Simplifies maintaining the doc commments.  But how does it affect the
> > > documentation generated from it?  Better, neutral, or worse?
> > 
> > Probably somewhere neutral.  There are definitely benefits, shorter
> >
> > man/html page in total, and avoid accidentally different docs over the same
> > fields.  E.g., we sometimes have different wordings for different objects:
> > 
> >        max-cpu-throttle
> >               maximum cpu throttle percentage.  Defaults to 99.  (Since 3.1)
> > 
> >        max-cpu-throttle: int (optional)
> >               maximum cpu throttle percentage.  The default value is 99. (Since 3.1)
> > 
> > This one is fine, but it's just very easy to leak in something that shows
> > differently.  It's good to provide coherent documentation for the same
> > fields over all three objects.
> 
> Do we have any actual problems though where the difference in
> docs is actively harmful ? I agree there's a possbility of the
> duplication being problematic, but unless its actually the
> case in reality, it is merely a theoretical downside.
> 
> IMHO for someone consuming the docs, this patch is worse, not neutral.

I agree.

> 
> > 
> > When looking at qemu-qmp-ref.7, it can be like this when we can invite the
> > reader to read the other section (assuming we only keep MigrationParameter
> > to keep the documentations):
> > 
> >    MigrationParameters (Object)
> > 
> >        The object structure to represent a list of migration parameters.
> >        The optional members aren't actually optional.  For detailed
> >        explanation for each of the field, please refer to the documentation
> >        of MigrationParameter.
> > 
> > But the problem is we always will generate the Members entry, where now
> > it'll all filled up with "Not documented"..
> > 
> >    Members
> >        announce-initial: int (optional)
> >               Not documented
> > 
> >        announce-max: int (optional)
> >               Not documented
> > 
> >        announce-rounds: int (optional)
> >               Not documented
> > 
> >        [...]
> > 
> > I think maybe it's better we just list the members without showing "Not
> > documented" every time for the other two objects.  Not sure whether it's an
> > easy way to fix it, or is it a major issue.
> > 
> > For developers, dedup the comment should always be a win, afaict.
> 
> IMHO that's optimizing for the wrong people and isn't a measurable
> win anyway. Someone adding a new parameter can just cut+paste the
> same docs string in the three places. It is a cost, but it is a
> small one time cost, where having "not documented" is a ongoing
> cost for consumers of our API.
> 
> I don't think the burden on QEMU maintainers adding new migration
> parameters is significant enough to justify ripping out our existing
> docs.

I had that strong feeling mostly because I'm a migration developer and
migration reviewer, so I suffer on both sides. :) I was trying to stand out
for either any future author/reviewer from that pov, but I think indeed the
ultimate consumer is the reader.

Fundamentally to solve the problem, maybe we must dedup the objects rather
than the documents only?  I'll try to dig a bit more in this area next week
if I have time, any link for previous context would be welcomed (or obvious
reason that we just won't be able to do that; I only know that at least
shouldn't be trivial to resolve).

For this one - Markus, let me know what do you think, or I'll simply repost
v3 with the duplications (when needed, probably later not sooner).

Thanks,

-- 
Peter Xu