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

deserialization should happen inside a transaction #38442

Merged
merged 1 commit into from
Feb 6, 2024
Merged

Conversation

piranha
Copy link
Contributor

@piranha piranha commented Feb 5, 2024

Deserialization can leave a target database in a broken state if an error happens. Wrapping it into transaction will prevent this.

Resolves #20632.

@piranha piranha added the no-backport Do not backport this PR to any branch label Feb 5, 2024
@piranha piranha requested a review from a team February 5, 2024 16:37
@piranha piranha self-assigned this Feb 5, 2024
@piranha piranha requested a review from camsaul as a code owner February 5, 2024 16:37
@piranha piranha added this to the 0.49 milestone Feb 5, 2024
@metabase-bot metabase-bot bot added the .Team/BackendComponents also known as BEC label Feb 5, 2024
Copy link

replay-io bot commented Feb 5, 2024

StatusComplete ↗︎
Commitbff3f39
Results
⚠️ 4 Flaky
2267 Passed

@crisptrutski
Copy link
Contributor

I've just got some non functional concerns / structural questions.

The first thing I noticed was a disparity in the level of abstraction where the transaction was started - one at the top of the stack in a handler, and the other a few calls deep into the call stack for command. Why have we not pushed it further down in the former or placed it higher up in the latter?

My preference is to push it as low down the stack as necessary - it's a low level detail and in generally we'll be looking back up the stack from low level database code or exceptions to check if we're in a transaction.

In the lower level case here, we're putting it around code to (1) set a caching scope, (2) load the yaml file, and then (3) load it. As far as I can see the former two do not interact with the database, and we can our transaction block down into metabase-enterprise.serialization.v2.load/load-database!, and this would cover both cases.

Even if we need it slightly higher up the stack I'd rather stick it in a function that takes a filepath and ingestion options, and DRYs up those three steps.

@qnkhuat
Copy link
Contributor

qnkhuat commented Feb 6, 2024

agree with Chris, this should be done within serdes somehow

@piranha
Copy link
Contributor Author

piranha commented Feb 6, 2024

I had some doubts about having with-cache outside with-transaction — but now that I'm thinking about it, it does not make any sense. So yeah, I'll push it down a little bit.

@piranha
Copy link
Contributor Author

piranha commented Feb 6, 2024

@crisptrutski @qnkhuat re-review please :)

@piranha piranha merged commit d5b923c into master Feb 6, 2024
105 checks passed
@piranha piranha deleted the serialization-tx branch February 6, 2024 16:37
@paoliniluis paoliniluis removed this from the 0.49 milestone Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/BackendComponents also known as BEC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to rollback changes in failed serialization load
4 participants