Global order writer: don't try to delete the current fragment if it had failed to be created. #4052
Conversation
…ailed to be created.
|
This pull request has been linked to Shortcut Story #27681: Clearly expose write-permissions failure. |
There was a problem hiding this comment.
I'd like to see a unit test for this hopefully replicating an access denied, but if it's just a directory that doesn't exist, that should be fine. The unit test should validate that the thrown exception text contains the right message. The test should hit all writers.
There was a problem hiding this comment.
Thanks for the PR @teo-tsirpanis, I do agree with you that FragmentMetadata should do creation-deletion of resources on disk, but this doesn't mean your solution here is hacky.
I like Luc's idea of adding an access denied test to cover this, but I'm ok without one as well if it turns out to be very complicated to do.
| const auto& uri = global_write_state_->frag_meta_->fragment_uri(); | ||
|
|
||
| // Cleanup the fragment we are currently writing. There is a chance that the | ||
| // URI is empty if creating the first fragment had failed. |
There was a problem hiding this comment.
s/first/latest, failure can happen later in the write process as well, it only happens that a permissions issue triggers a failure on the first fragment write.
KiterLuc
dismissed
their stale review
Filed a shortcut story to add a test later.
[SC-35061](https://app.shortcut.com/tiledb-inc/story/35061/add-tests-for-write-access-failures) See also #4052 (review). The test runs on non-Windows only. --- TYPE: NO_HISTORY
If during a global order write we fail to create a fragment directory, we throw and
GlobalOrderWriter::clean_uptries to delete the new fragment. But because we had not yet assigned a URI to a fragment, we try to delete the empty string and get anError: Unsupported URI scheme:on Unix andError: Failed to delete path ''; not a valid path.on Windows, masking the real issue.This PR modifies
clean_upto not delete the directory if it is empty, allowing the right error to surface. It's a bit hacky - ideallyFragmentMetadatawould be C.41 compliant and an empty URI would not be possible, but that would require more extensive refactorings and delay the bug fix. The other writers are not susceptible to this bug; they write only one fragment and their clean-up logic is simpler and takes care of that.Fixes #4041.
SC-27681
Verified on my machine that the right error message is displayed.
TYPE: BUG
DESC: Global order writer: don't try to delete the current fragment if it had failed to be created.