Kevin Wolf <kw...@redhat.com> writes:
> Am 03.11.2023 um 10:36 hat Markus Armbruster geschrieben:
>> Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru> writes:
>>
>> > On 11.10.23 13:18, Fiona Ebner wrote:
>> >> Am 10.10.23 um 19:55 schrieb Vladimir Sementsov-Ogievskiy:
>> >>> On 09.10.23 12:46, Fiona Ebner wrote:
>> >>>>
>> >>>> Initially, I tried to go for a more general 'job-change' command, but
>> >>>> I couldn't figure out a way to avoid mutual inclusion between
>> >>>> block-core.json and job.json.
>> >>>>
>> >>>
>> >>> What is the problem with it? I still think that job-change would be
>> >>> better.
>> >>>
>> >> If going for job-change in job.json, the dependencies would be
>> >> job-change -> JobChangeOptions -> JobChangeOptionsMirror -> MirrorCopyMode
>> >> query-jobs -> JobInfo -> JobInfoMirror
>> >> and we can't include block-core.json in job.json, because an inclusion
>> >> loop gives a build error.
>>
>> Let me try to understand this.
>>
>> Command job-change needs its argument type JobChangeOptions.
>>
>> JobChangeOptions is a union, and JobChangeOptionsMirror is one of its
>> branches.
>>
>> JobChangeOptionsMirror needs MirrorCopyMode from block-core.json.
>>
>> block-core.json needs job.json for JobType and JobStatus.
>>
>> >> Could be made to work by moving MirrorCopyMode (and
>> >> JobChangeOptionsMirror, JobInfoMirror) to job.json or some place that
>> >> can be included by both job.json and block-core.json. Moving the
>> >> type-specific definitions to the general job.json didn't feel right to
>> >> me. Including another file with type-specific definitions in job.json
>> >> feels slightly less wrong, but still not quite right and I didn't want
>> >> to create a new file just for MirrorCopyMode (and
>> >> JobChangeOptionsMirror, JobInfoMirror).
>> >> And going further and moving all mirror-related things to a separate
>> >> file would require moving along things like NewImageMode with it or
>> >> create yet another file for such general things used by multiple
>> >> block-jobs.
>> >> If preferred, I can try and go with some version of the above.
>> >>
>> >
>> > OK, I see the problem. Seems, that all requires some good refactoring. But
>> > that's a preexisting big work, and should not hold up your series. I'm OK
>> > to proceed with block-job-change.
>>
>> Saving ourselves some internal refactoring is a poor excuse for
>> undesirable external interfaces.
>
> I'm not sure how undesirable it is. We have block-job-* commands for
> pretty much every other operation, so it's only consistent to have
> block-job-change, too.
Is the job abstraction a failure?
We have
block-job- command since job- command since
-----------------------------------------------------
block-job-set-speed 1.1
block-job-cancel 1.1 job-cancel 3.0
block-job-pause 1.3 job-pause 3.0
block-job-resume 1.3 job-resume 3.0
block-job-complete 1.3 job-complete 3.0
block-job-dismiss 2.12 job-dismiss 3.0
block-job-finalize 2.12 job-finalize 3.0
block-job-change 8.2
query-block-jobs 1.1 query-jobs
I was under the impression that we added the (more general) job-
commands to replace the (less general) block-job commands, and we're
keeping the latter just for compatibility. Am I mistaken?
Which one should be used?
Why not deprecate the one that shouldn't be used?
The addition of block-job-change without even trying to do job-change
makes me wonder: have we given up on the job- interface?
I'm okay with giving up on failures. All I want is clarity. Right now,
I feel thoroughly confused about the status block-jobs and jobs, and how
they're related.
> Having job-change, too, might be nice in theory, but we don't have even
> a potential user for it at this point (i.e. a job type that isn't a
> block job, but for which changing options at runtime makes sense).
>
>> We need to answer two questions before we do that:
>>
>> 1. How much work would the refactoring be?
>>
>> 2. Is the interface improvement this enables worth the work?
>>
>> Let's start with 1.
>>
>> An obvious solution is to split JobType and JobStatus off job.json to
>> break the dependency of block-core.json on job.json.
>>
>> But I'd like us to investigate another one. block-core.json is *huge*.
>> It's almost a quarter of the entire QAPI schema. Can we spin out block
>> jobs into block-job.json? Moves the dependency on job.json from
>> block-core.json to block-job.json.
>
> It also makes job.json depend on block-job.json instead of
> block-core.json, so you only moved the problem without solving it.
block-job.json needs block-core.json and job.json.
job.json needs block-core.json.
No circle so far.
