backupccl: always track opened writer#104187
Conversation
Previously we would do things -- like wrap the opened writer with an encryption shim -- after opening the writer but before storing it in the sink. This could mean that if the sink opened a writer, but failed to save it, and was then closed it would fail to close the writer. This changes that, so that as soon as the writer is opened it is saved and then if it is later wrapped, saved again, to ensure that we cannot lose track of any successfully opened writer. Fixes: cockroachdb#103597. Release note: none.
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
stevendanna
left a comment
There was a problem hiding this comment.
Funzies.
I do wonder if we could write a unit test for this case, but I won't block on that since we have a failing test that this is fixing.
|
One tricky thing about a unit test is that crash only happens when using google cloud storage, which by default we'd never do during unit tests. I thought about adding my mutex-wrapped-map of open writers that I used to debug this to nodelocal or userfile so they could synthesize the same crash but not sure it is worth it? In any case, merging the fix now to unblock @renatolabs and will keep pondering if there is a better unit test. bors r+ |
|
Build succeeded: |
Previously we would do things -- like wrap the opened writer with an encryption shim -- after opening the writer but before storing it in the sink. This could mean that if the sink opened a writer, but failed to save it, and was then closed it would fail to close the writer. This changes that, so that as soon as the writer is opened it is saved and then if it is later wrapped, saved again, to ensure that we cannot lose track of any successfully opened writer.
Fixes: #103597.
Release note: none.