-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make auto ID row creation in parallel more robust. #13606
Changes from 2 commits
af51642
2b52c11
49fad46
99ecefa
7030925
d11a3d3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,11 @@ | ||
import { getRowParams } from "../../../db/utils" | ||
import { | ||
outputProcessing, | ||
processAutoColumn, | ||
processFormulas, | ||
} from "../../../utilities/rowProcessor" | ||
import { context, locks } from "@budibase/backend-core" | ||
import { | ||
Table, | ||
Row, | ||
LockType, | ||
LockName, | ||
FormulaType, | ||
FieldType, | ||
} from "@budibase/types" | ||
import { context } from "@budibase/backend-core" | ||
import { Table, Row, FormulaType, FieldType } from "@budibase/types" | ||
import * as linkRows from "../../../db/linkedRows" | ||
import sdk from "../../../sdk" | ||
import isEqual from "lodash/isEqual" | ||
import { cloneDeep } from "lodash/fp" | ||
|
||
|
@@ -151,30 +142,7 @@ export async function finaliseRow( | |
// if another row has been written since processing this will | ||
// handle the auto ID clash | ||
if (oldTable && !isEqual(oldTable, table)) { | ||
try { | ||
await db.put(table) | ||
} catch (err: any) { | ||
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 | ||
} | ||
) | ||
} else { | ||
throw err | ||
} | ||
} | ||
await db.put(table) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we actually need this? Are we not persisting the table already for the autoid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily. If the table doesn't have an auto ID column it won't have been persisted. |
||
} | ||
const response = await db.put(row) | ||
// for response, calculate the formulas for the enriched row | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,7 @@ describe("sdk >> rows >> internal", () => { | |
lastID: 1, | ||
}, | ||
}, | ||
_rev: expect.stringMatching("2-.*"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we're saving the table and updating the |
||
}, | ||
row: { | ||
...row, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided to remove this because the code handles it being |
||
}, | ||
}, | ||
}) | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided to up the number of rows to be a bit more sure we're avoiding conflicts. |
||
internalSdk.save(table._id!, row, config.getUser()._id) | ||
) | ||
) | ||
|
@@ -209,19 +209,21 @@ describe("sdk >> rows >> internal", () => { | |
}) | ||
|
||
const persistedRows = await config.getRows(table._id!) | ||
expect(persistedRows).toHaveLength(20) | ||
expect(persistedRows).toHaveLength(30) | ||
expect(persistedRows).toEqual( | ||
expect.arrayContaining( | ||
Array.from({ length: 20 }).map((_, i) => | ||
Array.from({ length: 30 }).map((_, i) => | ||
expect.objectContaining({ id: i + 1 }) | ||
) | ||
) | ||
) | ||
|
||
const persistedTable = await config.getTable(table._id) | ||
expect((table.schema.id as AutoColumnFieldMetadata).lastID).toBe(0) | ||
expect( | ||
(table.schema.id as AutoColumnFieldMetadata).lastID | ||
).toBeUndefined() | ||
expect((persistedTable.schema.id as AutoColumnFieldMetadata).lastID).toBe( | ||
20 | ||
30 | ||
) | ||
}) | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good cleanup :)