GH-38333: [C++][FS][Azure] Implement file writes#38780
Conversation
Tom-Newton
changed the title
|
I think this is ready for review apart from its currently based on top of #38773. Once that PR merges the diff will be more readable. |
|
OK. I'll merge #38773 right now. Please rebase on main. |
Tom-Newton
force-pushed
the
tomnewton/implement_azure_file_writes/GH-38333
branch
from
480be61 to
ed29906
Compare
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
(My review isn't completed yet. I'll continue later.)
cpp/src/arrow/filesystem/azurefs.cc
Outdated
There was a problem hiding this comment.
Do we need to check whether the new_block_id exists in block_ids_ or not?
There was a problem hiding this comment.
If we want to be 100% confident of avoiding clashes then yes but personally I think the current solution is a good compromise.
The risk should be zero when using OpenOutputStream because every block ID will be created by this same scheme, using monotonically increasing integers. The risk when using OpenAppendStream is that previously committed blocks used unusual names that might conflict. For example if some other writer committed one block named 00002-arrow then that would conflict after this writer appends 2 additional blocks, and cause a corrupt blob. I think this is extremely unlikely so personally I think this is a good option. Additionally OpenAppendStream is not implemented at all for S3 and GCS so presumably its not used much.
There was a problem hiding this comment.
OK. Could you describe the risk as a comment? If we find a real world problem with the risk, we can revisit the risk.
cpp/src/arrow/filesystem/azurefs.cc
Outdated
There was a problem hiding this comment.
Do these metadata replace the existing metadata? Should we merge with the existing metadata?
There was a problem hiding this comment.
Closing/flushing an append stream will always completely replace the old metadata. This is covered by the AzuriteFileSystemTest, TestWriteMetadata test which I largely copied from gcsfs_test.cc.
I don't feel strongly but I think this is a reasonable choice. I think if it did merge then there would be no way to remove metadata keys through the arrow file system. Also replacing is simpler to implement than merging.
github-actions
bot
added
awaiting changes
Tom-Newton
force-pushed
the
tomnewton/implement_azure_file_writes/GH-38333
branch
from
b04e14c to
0178660
Compare
github-actions
bot
added
awaiting changes
github-actions
bot
added
awaiting merge
github-actions
bot
added
awaiting changes
|
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit c1b12ca. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 14 possible false positives for unstable benchmarks that are known to sometimes produce them. |
### Rationale for this change Writing files is an important part of the filesystem ### What changes are included in this PR? Implements `OpenOutputStream` and `OpenAppendStream` for Azure. - Initially I started with the implementation from apache#12914 but I made quite a few changes: - Removed the different code path for hierarchical namespace accounts. There should not be any performance advantage to using special APIs only available on hierachical namespace accounts. - Only implement `ObjectAppendStream`, not `ObjectOutputStream`. `OpenOutputStream` is implemented by truncating the existing file then returning a `ObjectAppendStream`. - More precise use of `try` `catch`. Every call to Azure is wrapped in a `try` `catch` and should return a descriptive error status. - Avoid unnecessary calls to Azure. For example we now maintain the block list in memory and commit it only once on flush. apache#12914 committed the block list after each block that was staged and on flush queried Azure to get the list of uncommitted blocks. The new approach is consistent with the Azure fsspec implementation https://github.com/fsspec/adlfs/blob/092685f102c5cd215550d10e8347e5bce0e2b93d/adlfs/spec.py#L2009 - Adjust the block_ids slightly to minimise the risk of them conflicting with blocks written by other blob storage clients. - Implement metadata writes. Includes adding default metadata to `AzureOptions`. - Tests are based on the `gscfs_test.cc` but I added a couple of extra. - Handle the TODO(apacheGH-38780) comments for using the Azure fs to write data in tests ### Are these changes tested? Yes. Everything should be covered by azurite tests ### Are there any user-facing changes? Yes. The Azure filesystem now supports file writes. * Closes: apache#38333 Lead-authored-by: Thomas Newton <[email protected]> Co-authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
2 participants
Rationale for this change
Writing files is an important part of the filesystem
What changes are included in this PR?
Implements
OpenOutputStreamandOpenAppendStreamfor Azure.ObjectAppendStream, notObjectOutputStream.OpenOutputStreamis implemented by truncating the existing file then returning aObjectAppendStream.trycatch. Every call to Azure is wrapped in atrycatchand should return a descriptive error status.AzureOptions.gscfs_test.ccbut I added a couple of extra.Are these changes tested?
Yes. Everything should be covered by azurite tests
Are there any user-facing changes?
Yes. The Azure filesystem now supports file writes.