Closed Bug 1448044 Opened 2 years ago Closed Last year

kinto-storage-adapter.js chokes on large updates

Categories

(Toolkit :: Blocklist Implementation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Details

Attachments

(1 file, 1 obsolete file)

The kinto-storage-adapter sqlite3 backend for kinto.js builds a dynamic query for record preload. In operations involving large numbers of records, this query can exceed the maximum variable limit in sqlite3.
Assignee: nobody → mgoodwin
Comment on attachment 8961449 [details]
Bug 1448044 - kinto-storage-adapter.js chokes on large updates

https://reviewboard.mozilla.org/r/230194/#review235832

::: services/common/kinto-storage-adapter.js:279
(Diff revision 1)
> +      const limit = 100;
> +      let length = options.preload.length;
> +      let preloaded = {};
> +      let preload;
> +
> +      for (let remainder = options.preload; remainder.length > 0; length = remainder.length) {

µnit: Why not use a `while`, since the `update` part of this isn't really valuable?

::: services/common/kinto-storage-adapter.js:286
(Diff revision 1)
> +          preload = remainder.slice(0, limit);
> +          remainder = remainder.slice(limit, length);
> +        } else {
> +          preload = remainder;
> +          remainder = [];
> +        }

Why branch here? If the second argument is longer than the length of the array, it gets clamped to the length of the array (as far as I can tell).

::: services/common/kinto-storage-adapter.js:301
(Diff revision 1)
>  
> -      const preloaded = rows.reduce((acc, row) => {
> +        preloaded = Object.assign(preloaded, rows.reduce((acc, row) => {
> -        const record = JSON.parse(row.getResultByName("record"));
> +          const record = JSON.parse(row.getResultByName("record"));
> -        acc[row.getResultByName("record_id")] = record;
> +          acc[row.getResultByName("record_id")] = record;
> -        return acc;
> +          return acc;
> -      }, {});
> +        }, {}));

Why not use `preloaded` as the accumulator and get rid of the `Object.assign` call?
Attachment #8961449 - Attachment is obsolete: true
Attachment #8961449 - Flags: review?(eglassercamp)
Comment on attachment 8967344 [details]
Bug 1448044 - kinto-storage-adapter.js chokes on large updates

https://reviewboard.mozilla.org/r/236046/#review241778

::: services/common/kinto-storage-adapter.js:280
(Diff revision 1)
> +      let preload;
> +      let more = options.preload;
> +
> +      while (more.length > 0) {
> +        preload = more.slice(0, limit);
> +        more = more.length > limit ? more.slice(limit, more.length) : [];

nit: I think you can still just replace this with `more.slice(limit, more.length)` -- no conditional needed.
Attachment #8967344 - Flags: review?(eglassercamp) → review+
Comment on attachment 8967344 [details]
Bug 1448044 - kinto-storage-adapter.js chokes on large updates

https://reviewboard.mozilla.org/r/236046/#review241780


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/common/kinto-storage-adapter.js:273
(Diff revision 1)
>        // Preload specified records from DB, within transaction.
> +
> +      // if options.preload has more elements than the sqlite variable
> +      // limit, split it up.
> +      const limit = 100;
> +      let length = options.preload.length;

Error: 'length' is assigned a value but never used. [eslint: no-unused-vars]
Pushed by mgoodwin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef75b7ebedc3
kinto-storage-adapter.js chokes on large updates r=glasserc
https://hg.mozilla.org/mozilla-central/rev/ef75b7ebedc3
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: Blocklist Policy Requests → Blocklist Implementation
You need to log in before you can comment on or make changes to this bug.