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.

Sign up for GitHub

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

Jump to bottom

Sort out client.LensRegistry transaction stuff #1649

Closed
Tracked by #1002
AndrewSisley opened this issue Jul 18, 2023 · 0 comments · Fixed by #1737
Closed
Tracked by #1002

Sort out client.LensRegistry transaction stuff #1649

AndrewSisley opened this issue Jul 18, 2023 · 0 comments · Fixed by #1737
Assignees
@AndrewSisley
Labels
area/schema Related to the schema system code quality Related to improving code quality

Comments

@AndrewSisley
Copy link
Contributor

AndrewSisley commented Jul 18, 2023 •
edited
Loading

Part of #1002

Some of the funcs take a txn. This is out of step with the rest of client.Store, and is kind of incorrect, as store.LensRegistry() should really already be scoped to the store txn.
@AndrewSisley AndrewSisley added area/schema Related to the schema system code quality Related to improving code quality labels Jul 18, 2023
@AndrewSisley AndrewSisley mentioned this issue Jul 18, 2023
Open
@AndrewSisley AndrewSisley self-assigned this Jul 31, 2023
@AndrewSisley AndrewSisley added this to the DefraDB v0.7 milestone Jul 31, 2023
@AndrewSisley AndrewSisley mentioned this issue Jul 31, 2023
Merged
AndrewSisley added a commit that referenced this issue Aug 3, 2023
@AndrewSisley
fix: Improve the way migrations handle transactions (#1737)
9d75c45 
## Relevant issue(s)

Resolves #1649 #1592

## Description

Improves the way migrations handle transactions, as well as fixing a
couple of concurrency issues:

- Adds locks around the various registry properties, these maps can be
accessed concurrently and need to be protected.
- Removes the transaction continuity issue in the client.LenRegistry
interface, where db.LensRegistry() returns an object that does not
respect the transactionality of the parent store, and takes `txn`s as
input parameters to some of its functions. It does this by following the
same pattern as `db.db`. (#1649)
- Fixes the bugs in the lens package where migrations set were not
visible/accessible until after commit. They are now visible within the
transaction scope. (#1592)

It still does not provide transaction snapshot isolation, I see that
issue as relatively high effort low reward at the moment.
shahzadlone pushed a commit to shahzadlone/defradb that referenced this issue Feb 23, 2024
@AndrewSisley
fix: Improve the way migrations handle transactions (sourcenetwork#1737)
a832f04 
## Relevant issue(s)

Resolves sourcenetwork#1649 sourcenetwork#1592

## Description

Improves the way migrations handle transactions, as well as fixing a
couple of concurrency issues:

- Adds locks around the various registry properties, these maps can be
accessed concurrently and need to be protected.
- Removes the transaction continuity issue in the client.LenRegistry
interface, where db.LensRegistry() returns an object that does not
respect the transactionality of the parent store, and takes `txn`s as
input parameters to some of its functions. It does this by following the
same pattern as `db.db`. (sourcenetwork#1649)
- Fixes the bugs in the lens package where migrations set were not
visible/accessible until after commit. They are now visible within the
transaction scope. (sourcenetwork#1592)

It still does not provide transaction snapshot isolation, I see that
issue as relatively high effort low reward at the moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AndrewSisley AndrewSisley

Labels
area/schema Related to the schema system code quality Related to improving code quality
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Improve the way migrations handle transactions AndrewSisley/defradb
1 participant
@AndrewSisley