[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] migration: Allow user to specify migration available bandwid
From: |
Peter Xu |
Subject: |
Re: [PATCH] migration: Allow user to specify migration available bandwidth |
Date: |
Fri, 4 Aug 2023 10:02:54 -0400 |
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