Make auto ID row creation in parallel more robust. #13606
Conversation
| if (err.status === 409) { | ||
| // Some conflicts with the autocolumns occurred, we need to refetch the table and recalculate | ||
| await locks.doWithLock( | ||
| { | ||
| type: LockType.AUTO_EXTEND, | ||
| name: LockName.PROCESS_AUTO_COLUMNS, | ||
| resource: table._id, | ||
| }, | ||
| async () => { | ||
| const latestTable = await sdk.tables.getTable(table._id!) | ||
| let response = processAutoColumn(null, latestTable, row, { | ||
| reprocessing: true, | ||
| }) | ||
| await db.put(response.table) | ||
| row = response.row | ||
| } | ||
| ) |
There was a problem hiding this comment.
This locking is no longer required, and didn't work anyway. You'd need to wrap the entire row creation in this lock for it to consistently prevent 409s, and that could be prohibitively expensive.
| @@ -145,6 +145,7 @@ describe("sdk >> rows >> internal", () => { | |||
| lastID: 1, | |||
| }, | |||
| }, | |||
| _rev: expect.stringMatching("2-.*"), | |||
There was a problem hiding this comment.
Because we're saving the table and updating the _rev on it in our new getNextAutoId function, this needed adding to this test.
| @@ -199,7 +199,7 @@ describe("sdk >> rows >> internal", () => { | |||
| await internalSdk.save(table._id!, row, config.getUser()._id) | |||
| } | |||
| await Promise.all( | |||
| makeRows(10).map(row => | |||
| makeRows(20).map(row => | |||
There was a problem hiding this comment.
Decided to up the number of rows to be a bit more sure we're avoiding conflicts.
| @@ -189,7 +190,6 @@ describe("sdk >> rows >> internal", () => { | |||
| type: FieldType.AUTO, | |||
| subtype: AutoFieldSubType.AUTO_ID, | |||
| autocolumn: true, | |||
| lastID: 0, | |||
There was a problem hiding this comment.
Decided to remove this because the code handles it being undefined. May as well exercise that.
| throw err | ||
| } | ||
| } | ||
| await db.put(table) |
There was a problem hiding this comment.
Do we actually need this? Are we not persisting the table already for the autoid?
There was a problem hiding this comment.
Not necessarily. If the table doesn't have an auto ID column it won't have been persisted.
| if (err.status === 409) { | ||
| // Some conflicts with the autocolumns occurred, we need to refetch the table and recalculate | ||
| await locks.doWithLock( | ||
| { | ||
| type: LockType.AUTO_EXTEND, | ||
| name: LockName.PROCESS_AUTO_COLUMNS, | ||
| resource: table._id, | ||
| }, | ||
| async () => { | ||
| const latestTable = await sdk.tables.getTable(table._id!) | ||
| let response = processAutoColumn(null, latestTable, row, { | ||
| reprocessing: true, | ||
| }) | ||
| await db.put(response.table) | ||
| row = response.row | ||
| } | ||
| ) |
Description
I've noticed a few times in tests that if you create rows with auto ID columns in parallel, you quite often get
409 Conflicterrors.The reason this happens is because we store the next ID for a new row on the table document itself. When you create a new row, it takes that ID, increments it, saves it back, then uses it in the row. When creating rows in parallel, this read-save-use process is quite likely to conflict.
To make this more robust, I've introduced a function called
getNextAutoIdthat will attempt to increment the next ID on the table and save it. If it gets a conflict, it will try again up to 5 times. When it succeeds, the next ID is returned. If it doesn't succeed in 5 attempts an error is thrown.Because this is attempted 5 times and is done per-column, it makes it much less likely for a 409 to be returned to the user.