Skip to content
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

[RND-644] - Investigate alternatives to MongoDB backend read-for-write-locking #304

Merged
merged 25 commits into from
Nov 6, 2023

Conversation

DavidJGapCR
Copy link
Contributor

Description

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have signed all of the commits on this PR.
  • I have agreed to the Ed-Fi Individual Contributors' License
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DavidJGapCR DavidJGapCR marked this pull request as draft October 8, 2023 15:45
@DavidJGapCR DavidJGapCR force-pushed the RND-644 branch 2 times, most recently from 9be48c1 to 42e6039 Compare October 13, 2023 17:09
@DavidJGapCR DavidJGapCR marked this pull request as ready for review October 16, 2023 14:58
@DavidJGapCR DavidJGapCR changed the title [RND-644] - DRAFT - Investigate alternatives to MongoDB backend read-for-write-locking [RND-644] - Investigate alternatives to MongoDB backend read-for-write-locking Oct 16, 2023
@bradbanister
Copy link
Contributor

Please delete writeLockReferencedDocuments() from DB.ts and the commented out references to it

const concurrencyCollection: Collection<ConcurrencyDocument> = newClient
.db(databaseName)
.collection(CONCURRENCY_COLLECTION_NAME);
await concurrencyCollection.createIndex({ meadowlarkId: 1, documentUuid: 1 }, { unique: true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these need to be split into separate indexes, since sometimes you don't have the meadowlarkId. I also wondering if they need to be unique or not. I don't think so, so think about that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll add or modify the tests to see what I can conclude about this. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the tests I ran, I don't think we need to split the indexes. So, I am thinking we can keep it as it is.

/**
* Remove the locks on in-use meadowlark documents, both those being directly updated and those being referenced.
*/
export async function removeDocumentLocks(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this needs to be a separate function. I think we may be able to deleteMany right after we insertMany...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum... interesting idea, never thought of it. I'll give that a try and experiment with the tests.

@@ -0,0 +1,96 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some tests for the new function.


await concurrencyCollection.deleteMany(
{
$and: [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these $ands necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Now that we have just documentUuid, this and is not necessary.

} catch (e) {
expect(e).toMatchInlineSnapshot(
`[MongoServerError: WriteConflict error: this operation conflicted with another operation. Please retry your operation or multi-document transaction.]`,
expect(e.message).toContain(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's better to use expect(e).toMatchInlineSnapshot('') that way it can be automatically updated

@andonyns andonyns merged commit 5d07e64 into main Nov 6, 2023
13 checks passed
@andonyns andonyns deleted the RND-644 branch November 6, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants