[Performance] Reading object history is inefficient

[mmd]

Currently, the reverter plugin fetches each object version via a single select.

GET ​https://api.openstreetmap.org/api/0.6/node/3221532428/1
GET ​https://api.openstreetmap.org/api/0.6/node/1711424674/1
GET ​https://api.openstreetmap.org/api/0.6/node/5739339750/1
GET ​https://api.openstreetmap.org/api/0.6/node/1711397715/1
GET ​https://api.openstreetmap.org/api/0.6/node/1711397713/1
GET ​https://api.openstreetmap.org/api/0.6/node/3221532426/2
GET ​https://api.openstreetmap.org/api/0.6/node/26819461/8
GET ​https://api.openstreetmap.org/api/0.6/node/1711376838/4
GET ​https://api.openstreetmap.org/api/0.6/node/1711424666/1

This is quite inefficient to the fixed cost for each call. Luckily, the OSM API 0.6 offers a multi fetch call (see ​https://wiki.openstreetmap.org/wiki/API_v0.6#Multi_fetch:_GET_.2Fapi.2F0.6.2F.5Bnodes.7Cways.7Crelations.5D.3F.23parameters)

By using a multi fetch, the call above could be turned into a single call to fetch the respective object version

GET ​https://api.openstreetmap.org/api/0.6/nodes?nodes=3221532428v1,1711424674v1,5739339750v1,1711397715v1,1711397713v1,3221532426v2,26819461v8,1711376838v4,1711424666v1

[naoliv]

Probably the same fix (and same problem) from #11286

[anonymous]

Replying to naoliv:

Probably the same fix (and same problem) from #11286

Yes, it's the same topic. In the meantime, is has already been implemented in cgimap, only the Rails port is still missing (but not used anyway for this call): ​https://github.com/openstreetmap/openstreetmap-website/pull/1189

[mmd]

I'm increasing the priority of this issue to critical as a follow up for ​https://lists.openstreetmap.org/pipermail/talk-gb/2019-August/023327.html

This mapping accident which moved a large number of nodes (>100k) in London took several hours to revert, due to the slow download of object versions in JOSM reverter. Please consider switching to Multi Fetch (​https://wiki.openstreetmap.org/wiki/API_v0.6#Multi_fetch:_GET_.2Fapi.2F0.6.2F.5Bnodes.7Cways.7Crelations.5D.3F.23parameters) instead of downloading each object version one by one. Refer to API documentation for some example calls.

[mmd]

@steocker: why did you reduce the priority to normal? Being able to quickly repair bad edits is essential, now that the changeset upload is no longer handled by the Rails port.

[stoecker]

A bug which affects only few people and which is not causing a total failure is a normal priority.

[Upliner]

Fixed in [o35078]
PS: sorry for garbled commit message, I wasn't here for long time and forgot how to use ant publish

[GerdP]

I did not yet try it but I think the new code is not properly handling the case when one or more of the requested elements don't exist? See #17291 for my work on this in core.

[Upliner]

GerdP, see ChangesetReverter.java lines 247-259, I wrote them specifically for this issue. But I haven't tested this case either. If you have a changeset number where this issue happens, I will be able to test it.

[mmd]

Maybe one of the changesets including redactions would be useful here, like ​https://www.openstreetmap.org/user/pnorman%20redaction%20revert

I know that reverter plugin blacklists two known revertion users. For testing purposes you could try and disable this check in ChangesetReverter.java

[GerdP]

Replying to Upliner:

GerdP, see ChangesetReverter.java lines 247-259, I wrote them specifically for this issue. But I haven't tested this case either. If you have a changeset number where this issue happens, I will be able to test it.

Ah, okay, I didn't understand that this code handles the problem.
BTW: Great performance improvement! I tried to code that a few month ago for the core method and failed.

[Don-vip]

Replying to Upliner:

Fixed in [o35078]
PS: sorry for garbled commit message, I wasn't here for long time and forgot how to use ant publish

Thanks a lot for resuming your work on this plugin :)

[mmd]

Thanks a lot, performance improvement is truly impressive!