Repository navigation
Conversation
|
atiannicelli
left a comment
There was a problem hiding this comment.
I ran this end to end against PostGIS with v2's evolutions applied. These all behaved correctly: created/updated/unchanged/closed/reopened counts, case-insensitive id matching, the 422 errors (line, id and pointer, for invalid geometry and duplicate ids), 415 for the wrong content type, gzip bodies, and the 403 and 404 cases. #1300 statement triggers kept completion_metrics and the dirty tile cells in sync, and a 50,000-feature (6.4 MB) upload applied in 4.4s.
One transaction, a per-challenge lock, and validating everything before writing is a solid design. Most of the issues below are places where it doesn't yet match what v2's rebuild does, plus one cross-service risk.
Must fix: v2's cache can undo these writes
v2 caches challenges and tasks in memory by default (caching.type="caffeine", with expireAfterAccess, so entries that keep being read never expire). ChallengeDAL.update merges the request over the cached challenge and writes the whole row back, falling back to the cached overpass_ql, remote_geo_json, status and status_message (ChallengeDAL.scala). TaskDAL.update does the same for a task's name, geometry, priority and status (TaskDAL.scala). So after a v3 upload, an edit through v2 can silently restore the old remote GeoJSON URL, challenge status or task data. This needs either v2 running with MR_CACHING_TYPE=noop in production (documented as a deploy requirement for v3), or a way for v3 to invalidate v2's cache.
Should fix
- The upload SQL isn't checked. db/uploads.rs uses runtime sqlx::query, so unlike the query! macros it isn't verified against the schema at compile time, and there are no database tests. A few #[sqlx::test] cases (create, update, close, reopen, duplicate ids, invalid geometry) would cover the core path.
- Deploys abort uploads. DRAIN_TIMEOUT in lib.rs is 5s, but an upload's apply phase can run for up to 120s, so a deploy cuts off any upload in progress. It rolls back cleanly, but the client sees a reset after sending up to 100 MB. Consider a longer drain for upload routes.
The rest are line comments.
| Ok(sqlx::query( | ||
| "INSERT INTO tasks | ||
| (name, parent_id, instruction, status, priority, geojson, geom, location) | ||
| SELECT s.key, $1, '', 0, $2, s.geojson, s.geom, ST_Centroid(s.geom) |
There was a problem hiding this comment.
Must fix (or scope out explicitly): new tasks always get the challenge's default_priority ($2), and updated tasks keep whatever priority they had. v2 evaluates the challenge's high/medium/low priority rules for every task on each rebuild (Task.getTaskPriority, TaskDAL.scala; also updateTaskPriorities, ChallengeDAL.scala). Challenges that use priority rules will get wrong priorities after a v3 upload. Options: apply the rules here against the staged properties, reject challenges that have priority rules with a 409 like Overpass challenges, or document it as a known limitation.
There was a problem hiding this comment.
This is still up in the air, but most likely I will turn this into a known limitation (i.e., API v3 will initially ship without support for task priorities). Down the road, I'd rather implement this by having clients just upload a priority field on each task, rather than defining priorities as rules on the challenge which MapRoulette needs to evaluate on each task every time it's updated.
There was a problem hiding this comment.
That makes sense. I think either way works and if it is easier to not have the rules ont he server and just require a priority field then I'm down with that.
| let open = sqlx::query_scalar!( | ||
| r#" | ||
| WITH open AS ( | ||
| SELECT count(*) AS n FROM tasks WHERE parent_id = $1 AND status IN (0, 3, 6) |
There was a problem hiding this comment.
Must fix: this counts too-hard (6) as open. v2 counts only created (0) and skipped (3) when it decides between finished and ready (updateFinishedStatus, updateReadyStatus). I reproduced it: with only a too-hard task left, this sets the challenge to ready (3) where v2 sets finished (5), so the status will flip whenever the other service next touches the challenge. Using IN (0, 3) here would match v2.
There was a problem hiding this comment.
API v2 is inconsistent about whether or not tasks marked "Too Hard" (status 6) are counted as open or not. The mapping workflow treats them as open (GET /tasks/random will return them, they're included in the task map on the "Explore" page, and they can be bundled with other open tasks when mapping). But the challenge management endpoints treat "Too Hard" as a resolution, so a challenge with all of its tasks marked "Too Hard" is considered finished. This status also has two different names in the UI ("Can't complete" is the other one) which contributes to this confusion.
Making API v3 treat status 6 as open is a deliberate attempt to make this more consistent, at least until we can overhaul statuses at some later time (there are several others that have inconsistent meaning or are redundant)
| Ok(sqlx::query( | ||
| "UPDATE tasks t SET status = 9 | ||
| WHERE t.id IN (SELECT id FROM upload_locked) | ||
| AND t.parent_id = $1 AND t.status IN (0, 3, 6) |
There was a problem hiding this comment.
Must fix (same issue): this also closes too-hard tasks that are missing from the upload (and line 185 locks them). v2's removeIncompleteTasks only touches created and skipped tasks (ChallengeDAL.scala).
Consider, separately: v2 deletes those tasks, while this disables them (status 9). That's a reasonable choice since it keeps history and allows reopening, but disabled tasks pile up and count in the totals. A 50,000-feature upload followed by a one-feature upload left over 50,000 disabled tasks on the challenge. Worth documenting, or giving owners a way to purge them.
| -- this challenge into what v2 calls a "local geojson challenge" | ||
| -- (one created by direct upload) | ||
| overpass_ql = NULL, | ||
| remote_geo_json = NULL, |
There was a problem hiding this comment.
Must fix, or document: this silently turns a remote-GeoJSON challenge into a local one (I confirmed remote_geo_json is NULL after an upload). The description says remote GeoJSON isn't supported, but the endpoint accepts these challenges and wipes the URL, so the owner loses the source and v2's rebuild-from-URL stops working. Either reject them with a 409 like Overpass challenges, or make the conversion explicit in the docs and response.
There was a problem hiding this comment.
Hmm, yeah, that's an inconsistency in behaviour between Overpass QL and Remote GeoJSON challenges.
My goal is ultimately to deprecate Overpass QL and Remote GeoJSON challenge support in the server API, since both can be implemented on the client (for example, a client can easily fetch data from a remote URL and then upload that to the MapRoulette API). Overpass QL challenges would still be supported in the maproulette.org UI, but the query would be run from the browser. This simplifies the backend implementation and improves security (fetching and processing user-supplied URLs is a can of worms).
So the intent with the new PUT /api/v3/challenges/:id/tasks endpoint is that it converts legacy challenge types to the new form (what v2 calls "Local GeoJSON" challenges, i.e. where tasks are created directly from GeoJSON features uploaded by the user).
I added the HTTP 409 response for Overpass QL challenges because I realized that existing Overpass QL challenges use an integer ID for their task key, which does not uniquely identify the OSM element that the task is linked to. For example, a task might have ID 12345 if it corresponds to way/12345. API v3 expects tasks which are linked to OSM elements to provide keys in the latter form. So before I let the v3 endpoint "take over" Overpass QL challenges, I need to write a migration to fix existing challenges' IDs, otherwise the first time someone uses this endpoint to update a challenge that was created using Overpass, it'll close all of that challenge's open tasks and create new ones, instead of properly matching them.
Remote GeoJSON challenges don't have the same issue so I left them enabled in the PR as it currently stands, though I see now that the wording in the PR description I wrote (below) contradicts this:
This endpoint replaces the "local GeoJSON" challenge creation and update workflow from API v2. It does not support OverpassQL or remote GeoJSON challenge types.
What I should have said was that this endpoint does not intend to re-implement API v2's Overpass QL or Remote GeoJSON challenge types; in other words, uploading tasks by HTTP PUT with an NDJSON body will be the only way to "rebuild" a challenge in API v3. Users who want to can convert their challenges which were created under API v2 to this form by calling the API v3 endpoint.
There was a problem hiding this comment.
Well... that's a bummer. We use the remove GeoJson challenges all the time. When updating a challenge every week like we do it is much easier to:
- push data to geoJson file that in publicly available
- ping the challenge to rebuild the challenge
I can see your point that I can just implement an update process instead of a challenge rebuild API call, but the issue is that with lots of tasks the update can take a while and keep our client busy for that whole time.
If you must then you must, but I personally like the remote geoJson functionality.
There was a problem hiding this comment.
The rebuild operation should be a lot faster through the new API (a couple of seconds to rebuild a challenge with thousands of tasks, rather than several minutes) so that should hopefully help alleviate some of the pain.
| let (updated, reopened): (i64, i64) = sqlx::query_as( | ||
| "WITH changed AS ( | ||
| UPDATE tasks t SET | ||
| status = CASE WHEN c.reopen THEN 0 ELSE t.status END, |
There was a problem hiding this comment.
Should fix: reopening only sets status = 0. When v2 resets a task to created it also clears completed_time_spent, completed_by, completion_responses, is_bundle_primary, bundle_id and mapped_on (TaskDAL.scala). I confirmed a reopened task kept completed_by and mapped_on. v2's rebuild upsert also sets changeset_id = -1 and deletes task_review rows for tasks back at created (TaskDAL.scala). Bundled tasks aren't considered when closing either.
| max_errors: usize, | ||
| ) -> Result<UploadCounts, UploadError> { | ||
| db::uploads::raise_statement_timeout(conn).await?; | ||
| db::challenges::lock_upload(&mut *conn, challenge).await?; |
There was a problem hiding this comment.
Should fix: the per-challenge lock is taken only after the whole body has been received, and waits at most the session's lock_timeout (5s). A second upload to the same challenge (a retry, or another v3 instance) that arrives while one is applying gets a 503 after sending up to 100 MB. Consider checking the lock before receiving (e.g. pg_try_advisory_lock), or raising lock_timeout for this transaction to match the 120s statement timeout.
There was a problem hiding this comment.
I made the lock timeout fairly short because the request holds a database connection while it's waiting, so if it is very long it could be used as a denial of service vector. If a challenge owner sends two simultaneous requests to rebuild their challenge, I think it's fine if one of those requests fails (the backend doesn't need to queue and serialize the second request for them)
| fn default() -> UploadLimits { | ||
| UploadLimits { | ||
| max_bytes: 100 * 1024 * 1024, | ||
| max_features: 100_000, |
There was a problem hiding this comment.
Should fix: v2 caps challenges at 50,000 tasks (max_tasks_per_challenge, application.conf). Should this match, and be configurable?
There was a problem hiding this comment.
I bumped it because there are existing challenges which have >50,000 tasks somehow (I'd guess due to a bug where the limit is not being enforced in some rebuild path, but I'm not sure), and updating those challenges through API v3 would be impossible if the same limit were kept.
Configurability probably wouldn't hurt, though in practice there are very few third-party instances of MapRoulette and I don't know if any of their operators care about changing this particular limit.
| None => { | ||
| problems.push(problem( | ||
| "/id", | ||
| "is required, because it identifies the task", |
There was a problem hiding this comment.
Consider: v2 names tasks from the challenge's osmIdProperty, then id, @id, osmid, osm_id or name, falling back to a random UUID (ChallengeProvider.scala). Requiring a top-level id is cleaner, but the first v3 upload to a challenge whose tasks were named from properties or UUIDs will close every existing task and create new ones, so progress on them doesn't carry over. Worth calling out in the docs, or matching on osmIdProperty when the challenge has one.
There was a problem hiding this comment.
This is deliberate. I plan to write up a migration guide for challenge authors once this is deployed that describes the changes they need to make when producing challenge data in order to safely switch to the new API endpoint.
There was a problem hiding this comment.
OK., This is fine with me. the change it fine. We always delete unfinished tasks before we rebuild. So a new task is fine... the only thing that worries me is that is we have issues that someone marked as "not an issue" but we keep it in the dataset then it will go from being not an issue to a new issue with a new name that hasn't yet been completed? hmmm.
There was a problem hiding this comment.
the only thing that worries me is that is we have issues that someone marked as "not an issue" but we keep it in the dataset then it will go from being not an issue to a new issue with a new name that hasn't yet been completed?
If you keep the same key (i.e. the GeoJSON Feature's id), then a task that's marked as "not an issue" should keep that status even if you include it again in a subsequent PUT. Conceptually, the idea is that the endpoint updates task definitions (geometry, properties, in the future possibly priority, etc) but never changes the statuses that mappers set on the tasks. So hopefully if I've understood you correctly, this means it does what you hope it does by default.
The only "exception" is closed (status = 9) which isn't really a status; it should be thought of as more like a soft delete. Refactoring the current status concept in the future (possibly splitting it into status, i.e. is the task open or not, and resolution, i.e. what choice did the mapper make when they closed it) might help make this make more sense.
| SELECT s.line, s.key, '/geometry', | ||
| 'is invalid: ' || v.reason || coalesce(' at ' || ST_AsText(v.location), '') | ||
| FROM upload_staged s | ||
| CROSS JOIN LATERAL ST_IsValidDetail(s.geom) v |
There was a problem hiding this comment.
Consider: v2 repairs invalid geometry with ST_MakeValid (130.sql) and skips features it can't parse, while this rejects the whole upload. Strict is defensible for a new API, but scripts moving from v2 will hit it (e.g. self-intersecting polygons), so it's worth mentioning in the endpoint docs.
There was a problem hiding this comment.
Ditto above: this is a deliberate change to make the API stricter and reject invalid data rather than silently coercing it into shape.
|
My robot and I have been looking at this and have found two bigger things, pasting below. It tracked insert times and it all looks good, some additional findings are what Alex had mentioned and I removed those from the below. Just two new ones. How I tested. I ran v2 (
I counted every table (81) before and after each step, recorded the SQL each server logged, and diffed the tasks through the DB and through Timings (3 runs each)
The response counts were correct in every run: created 300, updated 500, unchanged 18,500, closed 1,000. Reopening the 1,000 closed tasks also worked. Must fix
Nits
|
|
And of course i did a stress to check bounds, looks good! The robot noticed a couple of minor things. I do wodner if Anyway, stress result! (so far i only did a 100% write, no mixed stuff (obviously because there's no read yet)): Stress test: concurrent challenge creates (
|
| Concurrent | HTTP results | Fastest / slowest upload | Last one done | Tasks/s overall | v3 DB connections (max / busy) |
|---|---|---|---|---|---|
| 1 | 1 × 200 | 1.49 s | 1.53 s | ~13k | 3 / 1 |
| 4 | 4 × 200 | 2.04 / 3.88 s | 3.92 s | ~20k | 6 / 2 |
| 8 | 8 × 200 | 1.93 / 7.12 s | 7.16 s | ~22k | 10 / 2 |
| 16 | 8 × 200, 8 × 503 | 1.98 / 7.22 s (the 200s) | 7.42 s | ~22k | 10 / 2 |
What I checked (all 29 challenges, 0 mismatches)
- Each of the 21 accepted uploads has exactly 20,000 open tasks, status READY (3), and a bounding box centred on its own location, so nothing landed in the wrong challenge.
- Each of the 8 rejected (503) challenges is untouched: no tasks, NULL status, NULL bounding.
- No duplicate task names within any challenge.
- No v3 errors other than the 8 expected rejections.
What this shows
- Uploads apply two at a time. Completion times come in pairs (about 2.0, 3.6, 5.4 and 7.1 s at 8 concurrent). That's
applying = 2: two uploads write at once and the rest queue in memory without holding a DB connection. Overall throughput levels off at about 22k tasks/s, and the slowest upload's time grows with roughly N/2 × 1.8 s. - Beyond 8, uploads are refused, not queued. At 16, exactly 8 got
503withRetry-After: 5and anapplication/problem+jsonbody, within about 20 ms. Clients have to retry those themselves. - The connection pool doesn't run out. v3 reached its 10-connection maximum, but at most 2 were ever busy. There were no pool-acquire timeouts, and the whole database peaked at 50 of its 100 connections.
Suggestions
- Log the 503 rejections at WARN or INFO instead of ERROR. They currently log as
ERROR request failed … too many uploads are being received, but this is intended load shedding. At ERROR, a burst of uploads looks like an outage in alerting. - Slow-statement warnings under normal load. With two uploads applying at once,
INSERT INTO taskstook 1.00–1.37 s and set off sqlx's default 1 s slow-statement warning 13 times. Raise the threshold for the upload transaction, or expect this noise in production. - Make the concurrency limits configurable.
receiving: 8andapplying: 2are hard-coded inconfig.rs; the code already notes they could come from env vars. Under load,applyingis what sets the latency, so it's the one most worth exposing. - Rejected uploads leave an empty challenge. After a 503 the challenge has no tasks and a NULL status, and v2's UI may show it as "not built" rather than failed. Document that clients must retry on 503, or have the frontend handle it.



Builds on top of #1307
This implements a new
PUT /api/v3/challenges/:id/tasksendpoint. The endpoint accepts a newline-delimited stream of GeoJSON Features, each one representing a task, and upserts those tasks into the challenge. You can call the endpoint after creating a challenge to populate it with tasks, or at any time later to update the existing tasks, remove old ones, add new ones, etc.The GeoJSON features are matched up with existing tasks by their
idproperty (which the API calls the task'skeyelsewhere to distinguish it from the integer ID assigned by MapRoulette).9). This is different from "Fixed"; it means the task was removed by the challenge manager.The endpoint is atomic: the entire upsert either succeeds or fails as one operation. It validates GeoJSON tasks one at a time in a stream as they are uploaded, and spools the results to a temporary file. Once the entire stream is uploaded, it acquires a database connection and applies the update in a transaction.
This endpoint replaces the "local GeoJSON" challenge creation and update workflow from API v2. It does not support OverpassQL or remote GeoJSON challenge types. It also does not support
cooperativeWork(Tag Fix or Cooperative challenge types).PATs with the
challenges:writescope can call this endpoint, so it can be used from third-party scripts.