On 29.07.21 18:29, Vladimir Sementsov-Ogievskiy wrote:
29.07.2021 16:47, Max Reitz wrote:
On 29.07.21 13:35, Vladimir Sementsov-Ogievskiy wrote:
29.07.2021 13:38, Max Reitz wrote:
On 29.07.21 12:02, Vladimir Sementsov-Ogievskiy wrote:
28.07.2021 10:00, Max Reitz wrote:
On 27.07.21 18:47, Vladimir Sementsov-Ogievskiy wrote:
Hi all!
That's an alternative to (part of) Max's
"[PATCH for-6.1? v2 0/7] mirror: Handle errors after READY cancel"
and shows' my idea of handling soft-cancelling READY mirror case
directly in qmp_block_job_cancel. And cleanup all other job
cancelling
functions.
That's untested draft, don't take it to heart :)
Well, I would have preferred it if you’d rebased this on top of
that series, precisely because it’s an alternative to only part
of it. And if it’s just an untested draft, that would have been
even better, because it would’ve given a better idea on what the
cleanup looks like.
There are also things like this series making cancel internally
always a force-cancel, where I’m not sure whether we want that in
the replication driver or not[1]. With my series, we add an
explicit parameter, so we’re forced to think about it, and then
in this series on top we can just drop the parameter for all
force-cancel invocations again, and for all non-force-cancel
invocations we would have to think a bit more.
I now don't sure that patch 5 of your series is correct (see my
last answer to it), that's why I decided to not base on it.
Well, we can always take patch 5 from v1. (Where I changed any
job_is_cancelled() to job_cancel_requested() when it influenced the
external interface.)
My series has the benefit of handling soft-mirror-cancel case the
other way and handles mirror finalization in case of soft-cancel
properly.
Specifically as for this series, I don’t like job_complete_ex()
very much, I think the parameter should be part of job_complete()
itself.
That was my idea. But job_complete is passed as function pointer,
so changing its prototype would be more work.. But I think it's
possible.
If we think that’s too specific of a mirror parameter to
include in normal job_complete(), well, then there shouldn’t be a
job_complete_ex() either, and do_graph_change should be a
property of the mirror job (perhaps as pivot_on_completion)
that’s cleared by qmp_block_job_cancel() before invoking
job_complete().
This way users will lose a way to make a decision during job
running..
On the contrary, it would be a precursor to letting the user change
this property explicitly with a new QMP command.
But probably they don't need actually. Moving the option to mirror
job parameter seems a good option to me.
Max
[1] Although looking at it again now, it probably wants
force-cancel.
What do you think of my idea to keep old bugs as is and just
deprecate block-job-cancel and add a new interface for
"no-graph-change mirror" case?
I don’t see a reason for that. The fix isn’t that complicated.
Also, honestly, I don’t see a good reason for deprecating anything.
Current interface lead to mess in the code, that's bad. Cancellation
mode that is actually a kind of completion (and having comments in
many places about that) - that shows for me that interface is not
good.. It's a question of terminology, what to call "cancel". Also,
that's not the first time this question arise. Remember my recent
cancel-in-flight-requests series, when I thought that "cancel is
cancel" and didn't consider soft-cancel of mirror.. And reviewers
didn't caught it. I don't think that interface is good, it will
always confuse new developers and users. But that's just my opinion,
I don't impose it )
If not deprecate, i.e. if we consider old interface to be good, than
no reason for this my series and for introducing new interface :)
I’m not against a better interface, I’m against using this current
bug as an excuse to improve the interface. We’ve known we want to
improve the interface for quite a long time now, we don’t need an
excuse for that.
If we use this bug as an excuse, I’m afraid of becoming hung up on
interface discussions instead of just getting the bug fixed. And we
must get the bug fixed, it’s real, it’s kind of bad, and saying “it
won’t appear with the new interface, let’s not worry about the old
one” is not something I like.
OTOH, if we use this bug as an excuse, I’m also afraid of trying to
rush the design instead of actually implementing the interface that
we’ve always desired, i.e. where the user gets to choose the
completion mode via yet-to-be-implemented some job property setter
function.
As a final note (but this is precisely the interface discussion that
I want to avoid for now), I said I don’t see a good reason for
deprecating anything, because `job-cancel force=false` can just
internally do `set-job-property .pivot_on_completion=false;
job-complete`. From an implementation perspective, that should be
simple.
I understand that for users the existence of the `force` flag may
still be confusing and so we might want to deprecate it for that
reason, but again, this is the interface discussion that I think we
don’t actually need right now.
And that’s why I’d prefer for any clean-up to come on top of fixing
the existing bug, so we can have separate discussions and take our
time with the interface without feeling driven by the need to fix a bug.
OK, I see your point. Let's just fix a bug with current interface and
anything else is a separate step. Are you going to send v3?
Yep, sure!
(Still want to look into the exact job->ret details, though. Perhaps I
can do better than just to keep it in the v1 state.)
Max