avniproject / avniproject/snapshot-server

Stop counting every record on every batch of a dump

Open
#5 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
0
Forks
0
PR merge metrics
No merged PRs in 30d

Description

### Motivation

Every time the dump generator asks avni-server for a batch of records, the server does two jobs. It fetches the batch. It also counts how many records exist in total, purely so the caller knows how many batches to expect.

The generator never needs that number. It reads batches until they run out.

The count is skipped on a routine incremental sync, because those come back short. A dump is a full pull from scratch, so it never comes back short — **every dump pays the count on every batch of every kind of record.**

Measured on prerelease during exactly this workload — a cold full pull:

| | requests | time in the database | read from disk |
|---|---|---|---|
| fetching the records | 629 | 1,057 s | 6,971 MB |
| **counting the records** | 220 | **895 s** | **4,442 MB** |

**46% of the database time and 39% of the disk reads, spent producing a number nobody uses.**

The disk reads matter more than the time. The page-size spike found that the generator can only run two large users at once — three is unreliable, six takes the host down — and that the cause is the sheer volume the database has to read, not the generator running out of processor or memory. It concluded that a bigger generator box would not help, and only more database memory or faster disks would. Removing two fifths of the reads is a third option nobody has costed.

This matters now because dump generation throughput is what sets how long a large organisation's switch to the new storage takes.

Part of avniproject/avni-client#1942. **Phase 1** — needed for the 18.0 storage switch.

---

### Acceptance Criteria

| | Today | Required |
|---|---|---|
| Database work per batch | fetches the batch **and** counts the whole set | fetches the batch only |
| How the generator knows to stop | reads the total batch count from the first response | follows the server's own "is there more" answer |
| Records in a finished dump | all of them | **unchanged — all of them** |
| A dump containing only part of the data | would report success | impossible, or fails loudly |

**Which side moves:** the database stops counting. The number of records in a finished dump must not move. A dump that got faster by fetching less has broken the thing this card protects.

- [ ] A dump for a user with several batches of records contains the same number of records, per kind of record, as a dump generated before this change. Compare the two files directly — the repo already has a comparison script for this.
- [ ] No count query runs during a dump. Confirm from the database's own statement statistics during a run, not from reading the code.
- [ ] A dump for a user whose records all fit in one batch is unchanged.
- [ ] A dump that fails part-way still resumes from where it stopped, without re-fetching everything and without skipping records.
- [ ] Wall-clock time and disk reads for one large user are recorded before and after, on the same box and the same server settings, so the gain is a measurement rather than an expectation.

---

### Tech Approach

**Why this is small here.** The device has to solve a progress-bar problem alongside this; the generator does not. `SyncRunner.js:138,140` passes `noop` for both `onProgressPerEntity` and `updateProgressSteps`, so nothing here consumes `totalPages` for display. The change is the loop and the URL, nothing else.

**The loop.** `src/rest/ConventionalRestClient.js:109` drives paging from `page.totalPages`:

```js
_.range(1, page.totalPages, 1).forEach(pageNumber => chainedRequests.push(...))
```

A `Slice` response has no `page` key, so `_.range(1, undefined)` yields `[]` — changing the URL without changing the loop **truncates every entity to its first batch and completes without error.** This is the main risk in the change.

**What a Slice response looks like.** `SlicedResources` (`avni-server/.../web/response/slice/SlicedResources.java:62`) puts its metadata under **`slice`**, not `page`:

```json
{ "_embedded": { "programEncounter": [ … ] },
"slice": { "size": 1000, "number": 0, "hasNext": true } }
```

Rows still arrive under `_embedded.`, so `Persister` and the hydration path are untouched. **Drain on `hasNext`.** Do not stop on a short batch instead — avni-server silently clamps `size` to 1000, so a request for 2000 is served 1000 and a short-batch test would stop after one. `hasNext` is immune to that.

**The URL.** Slice endpoints are a path *suffix*: `/individual/v2`, `/programEncounter/v2`. `getAllForEntity` (`src/rest/ConventionalRestClient.js:66-75`) places `apiVersion` as a *prefix*, giving `${serverURL}/v2/${resource}` — which reaches metadata endpoints like `/v2/dashboard` but cannot produce `/individual/v2`. A suffix mechanism is needed.

**Do not set `resourceUrl: "individual/v2"`.** `resourceUrl` is also the push URL upstream (`avni-client .../ConventionalRestClient.js:32-34`). The generator does not push, so it would appear to work here and then break avni-client when the file is re-vendored.

**Server side is already done.** 21 sync controllers expose a `/v2` Slice variant via `ScopeBasedSyncService.*AsSlice`: Individual, Encounter, ProgramEnrolment, ProgramEncounter, Checklist, ChecklistItem, Comment, CommentThread, GroupSubject, IndividualRelationship, Task, TaskUnAssignment, News, SubjectMigration, SubjectProgramEligibility, EntityApprovalStatus, IdentifierAssignment, UserSubjectAssignment, Session, AttendanceRecord, UserInfo. Confirm every entity in `EntityMetaData.model()` that the generator pulls has one; anything missing needs an avni-server card.

**Vendoring.** `src/rest/ConventionalRestClient.js` is copy-pasted from avni-client (`README.md:53-67`). This change diverges from upstream until avni-client#2096 lands the same loop. Mark it with the `…` convention already used in `src/rest/requests.js:27,43,54,84`, and add a row to the vendored-modules table saying what diverged and why.

**Resumption is unaffected.** `entity_sync_status` holds no batch number — resumption is by timestamp watermark, and `SyncRunner`'s `persistAll` advances it after every batch, not when the entity finishes.

---

### Testing Gotchas

- **Use a user with several batches of records.** With a user whose records fit in one batch of each kind, the count never fires and the failure this change risks introducing cannot appear.
- **A truncated dump looks like a successful one.** No error, no crash. Comparing record counts against a known-good dump is the only way to see it, which is what the repo's comparison script is for.
- **There is effectively no automated safety net here.** The repo has a single test and it does not touch the fetching loop; the build pipeline still reports that no tests are configured. Validation is by running a dump and comparing it against a known-good one.
- **Measure with one user at a time.** Two concurrent large users is the reliable ceiling and three is a coin-flip, so any concurrency above one adds more variance than the effect being measured.
- **Database caching swings these numbers by up to two times.** Run the before and after back to back, same user, same session. Two runs hours apart are not comparable.
- **Do not measure through an SSH tunnel.** The page-size spike found the tunnel was 83–98% of measured time and produced a conclusion that turned out to be pure artefact. Measure from inside the VPC.
- **The gain may not add to the batch-size change.** A larger batch size means fewer batches, which already means fewer counts — so part of the measured 2.1× from raising the batch size to 2000 is probably count reduction that this card removes anyway. **Measure the two together, not as separate wins stacked.**

---

### Out of Scope

- **The same change on the device.** Tracked as avniproject/avni-client#2096. The device also has to decide what its progress bar shows, which is why it is a separate, later card.
- **Raising the batch size from 1000 to 2000.** Separate card; see the sync page-size stories in avni-product-ops. Do them close together and measure once, per the note above.
- **Raising the number of users generated at once.** This card reduces the read volume that sets that ceiling, but it does not add the admission control the generator still lacks. That remains its own card.
- **Changing how the server addresses batches** so later batches stop costing more than earlier ones. A real further improvement, but it must be measured before it is committed to.

---

### Related

- avniproject/avni-client#2096 — the same change on the device
- avniproject/avni-client#1942 — parent card
- avniproject/avni-client#1915 (closed) — the page-size spike; source of the concurrency and batch-size findings

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in src/rest/ConventionalRestClient.js, especially getAllForEntity and the paging loop, then inspect SyncRunner.js:138,140 and the SlicedResources response shape. Run a multi-batch dump and compare it with a known-good dump using the repository's comparison script. Done means all records are preserved, paging follows slice.hasNext, no count query appears in database statement statistics, and before/after performance is measured under matching conditions.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
api, backend, data
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
57/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.