( )⚙ D8480 bundle: optional advisory obsolescence parts

This is an archive of the discontinued Mercurial Phabricator instance.

bundle: optional advisory obsolescence parts
ClosedPublic

Authored by joerg.sonnenberger on Apr 24 2020, 11:23 AM.

Details

Summary

It is useful to ship obsolescence markers as part of clonebundles or
pullbundles, but they shouldn't stop a non-evolution client from working.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

marmoute requested changes to this revision.Apr 24 2020, 11:28 AM
marmoute added a subscriber: marmoute.

We could add an option to make them optional when bundling, but we should not make it so by default. This code is triggered because the obsmarkers part have been explicitely requested, marking the part as optional seems wrong.

This revision now requires changes to proceed.Apr 24 2020, 11:28 AM

The main motivation for this change is to allow clonebundles and the like to properly seed the OBS exchange. I consider OBS markers optional metadata and the current handling of the exchange agrees with that. As such, it should be up to the consumer of the bundle to decide whether they can or want to do something useful with that data. As such, I consider this a step toward being able to enable the option by default.

My point is that it would be fine to make it optional for the bundle generated for clone/pull bundle. But in the general exchange case, is the client explicitly requested markerss, serving them is not optional.

This patch is contentious, but I'm a little unclear why. Summarizing so everyone at least parses my thoughts roughly the way I mean: mandatory parts mean "fail if you can't process this part." It seems basically reasonable to me that a client can choose to ignore (perhaps through ignorance) obsolete markers and it'll be potentially confusing, but not /wrong/ in that they'll have the changesets they expected, but also didn't lose some the server thought they would. It feels like a bug if a client requests obsmarkers and then discards them? But in the case of any kind of pre-computed bundle it's a little trickier because if the server (as an optimization, in this case) emits obsmarkers that the client didn't request, it could break old clients.

Given that obsmarkers are still off by default and effectively experimental, we can change whatever we want. So the question is: should we, and if so, should it be all obsmarker parts or only precomputed ones? I think it's pretty clear that we should at least allow optional obsmarker parts for precomputed bundles, and I think we also should allow mandatory obsmarker parts (so if you try to push markers to a server that doesn't grok them you get an error instead of ninja-duplicating some changesets with their evolved counterparts.)

So that leaves us with the one remaining question: what should the server do when the client proactively requests markers? I think in that case it basically doesn't matter, so either way is fine. If it's less work, I'm inclined to land the patch as-is, and we can do a follow-up if there's a bug case that I'm not understanding today.

TL;DR; We should add a third value to the bundlespec argument: hg bundle -t v2;obsolescence=optional

about part in general

Parts that are mandatory are parts that most be processed for the request (eg: push) or reply (eg: pull) to correct. Such parts are either verification steps or core payload that contains the content that was requested to be transfered.

Advisory parts carries information that will helps the applying side, but can be ignored without compromising the "result" of the bundle application.

about push/pull

Most exchanges happens through push and pull in these cases the two peers can negociate contents and capabilities, and the bundling side knows: (1) what is is requested to be exchanged (2) what the receiving side is expected to support. Requested content (eg: changesets, phases, obsmarkers) are sent in mandatory parts, because failure to transfer them would mean the failure of the exchange. The same a push not sending the changeset it should should be a failure, a push not sending the markers it should, should be a failure. Not sending markers result in changeset not being obsoleted and uter confusion for the user.

So the markers must be sent in a mandatory part during push/pull (same as the other important payload). If the receiving peer get a part version it does not understant, there is a bug, and it should bark.

about offline bundle

The case of offline bundle is a bit tricker. Two people might be exchanging data on purpose, expecting the obsmarker to be exchanged, or we can be in a blind case were data is pre-generated for all.

In the first case, we want the part to be mandatory, because the users wants the markers to be exchanges. In the second case, the server operator only want a best effort approach were users who do not care about the markers should not take them.

Possible way forward.

We already have a bundlespec argument to control obsmarkers inclusion:

hg bundle -t v2;obsolescence=yes  # with obsmarker
hg bundle -t v2;obsolescence=no   # without obsmarkers

It seems simple to add a third value, to include them as advisory:

hg bundle -t v2;obsolescence=optional   # without obsmarkers

Server operator could that option to pregenerate bundles.

joerg.sonnenberger retitled this revision from bundle2: make obsolescence parts optional to bundle: make obsolescence parts optional.May 15 2020, 9:33 AM
joerg.sonnenberger edited the summary of this revision. (Show Details)
joerg.sonnenberger updated this revision to Diff 21391.

While I don't really agree with the design interpretation of why the server should send mandatory, I don't care enough in this case. With the update, bundle and pull/push are getting different flags.

While I don't really agree with the design interpretation of why the server should send mandatory, I don't care enough in this case.

The is no "design interpretation going on here", am I the author of these APIs/protocols and I am clarifying their semantics. The intend for this part to be mandatory in the context have been here since the begining.

With the update, bundle and pull/push are getting different flags.

As I pointed out in my previous reply, there are valid usecase for having the part mandatory in a bundle too. Can you apply me recommendation and add a third values to the existing bundle-spec parameter for your usecase?

This patch is contentious, but I'm a little unclear why. Summarizing so everyone at least parses my thoughts roughly the way I mean: mandatory parts mean "fail if you can't process this part." It seems basically reasonable to me that a client can choose to ignore (perhaps through ignorance) obsolete markers and it'll be potentially confusing, but not /wrong/ in that they'll have the changesets they expected, but also didn't lose some the server thought they would. It feels like a bug if a client requests obsmarkers and then discards them? But in the case of any kind of pre-computed bundle it's a little trickier because if the server (as an optimization, in this case) emits obsmarkers that the client didn't request, it could break old clients.

Given that obsmarkers are still off by default and effectively experimental, we can change whatever we want. So the question is: should we, and if so, should it be all obsmarker parts or only precomputed ones? I think it's pretty clear that we should at least allow optional obsmarker parts for precomputed bundles, and I think we also should allow mandatory obsmarker parts (so if you try to push markers to a server that doesn't grok them you get an error instead of ninja-duplicating some changesets with their evolved counterparts.)

So that leaves us with the one remaining question: what should the server do when the client proactively requests markers? I think in that case it basically doesn't matter, so either way is fine. If it's less work, I'm inclined to land the patch as-is, and we can do a follow-up if there's a bug case that I'm not understanding today.

While I don't really agree with the design interpretation of why the server should send mandatory, I don't care enough in this case.

The is no "design interpretation going on here", am I the author of these APIs/protocols and I am clarifying their semantics. The intend for this part to be mandatory in the context have been here since the begining.

Given that we've got at least two readers that misinterpreted your intent, please send a clarifying patch to the protocol docs (along with rationale, preferably) so the intent is more clear? I think that might also be a good place to discuss what the semantics _should_ be, in case people have feelings that merit further discussion.

This patch is contentious, but I'm a little unclear why. Summarizing so everyone at least parses my thoughts roughly the way I mean: mandatory parts mean "fail if you can't process this part." It seems basically reasonable to me that a client can choose to ignore (perhaps through ignorance) obsolete markers and it'll be potentially confusing, but not /wrong/ in that they'll have the changesets they expected, but also didn't lose some the server thought they would. It feels like a bug if a client requests obsmarkers and then discards them?

I guess the misundestanding is here. Not exchanching markers when we should give wrong result and user activelly complains and report bugs then it happens to them. Hooks and checks can also get confused by the broken end-state this creates.

But in the case of any kind of pre-computed bundle it's a little trickier because if the server (as an optimization, in this case) emits obsmarkers that the client didn't request, it could break old clients.

In the case of precomputed bundled explicitly used as cache (pull bundle/clone bundke9, having the markers optional seems fine since the client will performs a full pull afterward, pull markers they might have missed earlier. My proposal of having a bundlespec option for this case hg bundle -t v2;obsolescence=optional should be covering that case, unless I am missing something.

In the other case of people using bundles to exchange data (eg: by email or with USB dongle). Making the part optionnal can lead to people not exchanging what they want, leading to wrong result, confusion and bug report/perception.

Given that obsmarkers are still off by default and effectively experimental, we can change whatever we want. So the question is: should we, and if so, should it be all obsmarker parts or only precomputed ones? I think it's pretty clear that we should at least allow optional obsmarker parts for precomputed bundles, and I think we also should allow mandatory obsmarker parts (so if you try to push markers to a server that doesn't grok them you get an error instead of ninja-duplicating some changesets with their evolved counterparts.)
So that leaves us with the one remaining question: what should the server do when the client proactively requests markers? I think in that case it basically doesn't matter, so either way is fine. If it's less work, I'm inclined to land the patch as-is, and we can do a follow-up if there's a bug case that I'm not understanding today.

I don't undestand your point here. If a client request obsmarkers and does not (effectively) receive them, the result if wrong, the end state can ge broken, user get confused and report/percieve bugs.

baymax requested changes to this revision.Nov 28 2020, 7:32 AM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Nov 28 2020, 7:32 AM
joerg.sonnenberger retitled this revision from bundle: make obsolescence parts optional to bundle: optional advisory obsolescence parts.Dec 3 2020, 4:30 PM
joerg.sonnenberger edited the summary of this revision. (Show Details)
joerg.sonnenberger updated this revision to Diff 24007.
marmoute requested changes to this revision.Dec 16 2020, 9:35 AM

This looks overall good. I have a couple of API and naming feedback.

mercurial/bundle2.py
1865

This should get a "True" defautl value to avoid unnecessary impact on the rest of the codebase.)

mercurial/commands.py
1651

That should probably be evolution.bundle-obsmarker.mandatory to make it a sub-option, or maybe even evolution.bundle-obsmarker:mandatory since the suboption cannot "live" without the super option.

I would also put this as a sub-conditionnation of the 'evolution.bundle-obsmarker' one right above.

(final small nits, negative logic give people headache, so maybe simply do bundlespec.contentopts[b'obsolescence-mandatory'] = bundlespec.contentopts[b'obsolescence-mandatory'] in multiple line)

This revision now requires changes to proceed.Dec 16 2020, 9:35 AM
joerg.sonnenberger marked an inline comment as done.Dec 16 2020, 11:00 AM
joerg.sonnenberger added inline comments.
mercurial/bundle2.py
1865

My preference here is to be explicit as it is not clear from the caller otherwise why something is mandatory or not?

marmoute requested changes to this revision.Dec 16 2020, 11:03 AM
marmoute added inline comments.
mercurial/bundle2.py
1865

obsmarker are store data that we must be mandatory in the vast majority of case. They are similar to changegroup data for example which don't requires the mandatory parameter to be explicilty passed all the time.

This revision now requires changes to proceed.Dec 16 2020, 11:03 AM
marmoute added inline comments.Dec 16 2020, 11:53 AM
mercurial/commands.py
1652–1663

I find this harder to read than the previous code. We got a 12 ling one-liner instead of simple if cond: \n logic

mercurial/commands.py
1652–1663

I blame black and the long names for that, mostly. It's a dict literal spread over 9 lines for 3 entries. No logic even. I don't want move the strings to a variable to make shorter, because it also makes it less grepable.

marmoute added inline comments.Dec 17 2020, 5:22 AM
mercurial/commands.py
1652–1663

What about:

# support the necessary features.
bundlespec.contentopts[b'obsolescence'] = ui.configbool(
    b'experimental',
    b'evolution.bundle-obsmarker'
)
bundlespec.contentopts[b'obsolescence-mandatory'] = ui.configbool(
    b'experimental',
    b'evolution.bundle-obsmarker:mandatory'
)
bundlespec.contentopts[b'bundle-phases'] = ui.configbool(
    b'experimental',
    b'bundle-phases'
)

Or maybe even:

opts = bundlespec.contentopts
cfg = lambda key: ui.configbool(b'experimental', key)
opts[b'obsolescence'] = cfg(b'evolution.bundle-obsmarker')
opts[b'obsolescence-mandatory'] = cfg(b'evolution.bundle-obsmarker:mandatory')
opts[b'bundle-phases'] = cfg(b'bundle-phases')

?

pulkit added a subscriber: pulkit.Dec 18 2020, 2:53 PM

Reading through the discussion which happened, it seems to me that the commit description needs to explain in a bit more detail and reasoning about what's happening in this patch and why we are not doing things in other ways.

marmoute accepted this revision.Dec 18 2020, 3:09 PM

thanks for the update

pulkit accepted this revision.Dec 21 2020, 6:50 AM
This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.