-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[ntuple] (WIP) RNTuple S3 object store backend #8525
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
base: master
Are you sure you want to change the base?
Conversation
Add a new page source and sink that use S3 as the backing store. Davix is used as the S3 interface. The implementation is nearly identical to the DAOS backend and there is a lot of duplicated code. Like the current DAOS backend, one object is allocated for every page, plus three for the header, footer, and anchor. Performance will not be very good yet as only a single request at a time is issued. Pages are issued keys sequentially from 0, like the DAOS backend. There are three reserved keys: * anchor: u64(-1) * header: u64(-2) * footer: u64(-3) S3 access is controlled using the (ROOT & Davix-compatible) envvars: * S3_REGION * S3_SECRET_KEY * S3_ACCESS_KEY Perhaps these should be changed to the official AWS envvars.
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests:
|
|
Build failed on mac11.0/cxx17. Failing tests: |
This lets users pass in either s3://bucket.s3_host or s3://bucket.s3_host/ for the location and my_ntuple or my_ntuple/ for the RNTuple name.
|
Starting build on |
|
Build failed on mac11.0/cxx17. Failing tests: |
jalopezg-git
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| # Enable RNTuple support for S3 backend through Davix | ||
| if(davix OR builtin_davix) | ||
| set(ROOTNTuple_EXTRA_HEADERS ${ROOTNTuple_EXTRA_HEADERS} ROOT/RPageStorageS3.hxx) | ||
| target_sources(ROOTNTuple PRIVATE v7/src/RPageStorageS3.cxx) | ||
| target_compile_definitions(ROOTNTuple PRIVATE R__ENABLE_DAVIX) | ||
|
|
||
| target_include_directories(ROOTNTuple PRIVATE ${DAVIX_INCLUDE_DIR}) | ||
| target_link_libraries(ROOTNTuple PRIVATE ${DAVIX_LIBRARY}) | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we can foresee the development of other backends in the future, we might want to have a better way of specifying which backends get built, e.g. -Drntuple_opt_backends=daos,s3 (the file backend should probably be always built).
Perhaps we could discuss the advantages at some point.
| #else | ||
| throw RException(R__FAIL("This RNTuple build does not support DAOS.")); | ||
| #endif | ||
|
|
||
| if (location.find("s3://") == 0) { | ||
| #ifdef R__ENABLE_DAVIX | ||
| return std::make_unique<RPageSourceS3>(ntupleName, location, options); | ||
| #else | ||
| throw RException(R__FAIL("This RNTuple build does not support S3.")); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: if we are going to support many backends in the future, maybe we should make the (conditional) construction of the RPageStorageXxx a bit more elegant.
| #else | ||
| throw RException(R__FAIL("This RNTuple build does not support DAOS.")); | ||
| #endif | ||
| } else if (location.find("s3://") == 0) { | ||
| #ifdef R__ENABLE_DAVIX | ||
| return std::make_unique<RPageSinkS3>(ntupleName, location, options); | ||
| #else | ||
| throw RException(R__FAIL("This RNTuple build does not support S3.")); | ||
| #endif | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (see above).
| const char *s3_sec = getenv("S3_SECRET_KEY"); | ||
| const char *s3_acc = getenv("S3_ACCESS_KEY"); | ||
| if (s3_sec && s3_acc) { | ||
| fReqParams.setAwsAuthorizationKeys(s3_sec, s3_acc); | ||
| } | ||
| const char *s3_reg = getenv("S3_REGION"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jblomer, maybe we should discuss if environment variables is the right way to pass this information?
| namespace Experimental { | ||
| namespace Detail { | ||
|
|
||
| class RS3Handle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change the name of the class, e.g. RS3Bucket, given that it provides read/write access to object in a bucket.
| // Danger: there are no size limits on the amount of data read into the buffer | ||
| try { | ||
| obj.get(nullptr, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure if it is supported by Davix S3 implementation, but we could use DavFile::readPartial() for this.
| std::vector<char> buf; | ||
| buf.reserve(RS3NTupleAnchor::GetSize()); | ||
| fS3Handle->ReadObject(kOidAnchor, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we happen to use DavFile::readPartial(), we should be able to provide a plain char[] buffer here.
(The same applies for the rest below).
| std::vector<char> buf; | ||
| buf.reserve(bytesOnStorage); | ||
| fS3Handle->ReadObject(std::to_string(pageInfo.fLocator.fPosition), buf); | ||
| R__ASSERT(buf.size() == bytesOnStorage); | ||
| directReadBuffer = std::make_unique<unsigned char[]>(bytesOnStorage); | ||
| memcpy(directReadBuffer.get(), buf.data(), bytesOnStorage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment above.
|
|
||
| TEST(RNTuple, S3Basics) | ||
| { | ||
| std::string s3Uri("s3://ntpl0.s3.us-east-2.amazonaws.com"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the test failing in CI because we are not setting the S3_xxx environment variables?
|
Davix apparently also supports Azure to some degree: https://davix.web.cern.ch/davix/docs/master/cloud-support.html#microsoft-azure. If there's an implementation that works for S3, Azure support might be as straightforward as swapping out the S3 setup steps for Azure setup. But this is scope creep for this PR and I don't know if Azure is a desired feature for users, etc. |
Add a new page source and sink that use S3 as the backing store. Davix
is used as the S3 interface. The implementation is nearly identical
to the DAOS backend and there is a lot of duplicated code.
results in the following objects stored in the bucket:
Like the current DAOS backend, one object is allocated for every page,
plus three for the header, footer, and anchor. Performance will not be
very good yet as only a single request at a time is issued.
Pages are issued keys sequentially from 0, like the DAOS backend. There
are three reserved keys:
S3 access is controlled using the (ROOT & Davix-compatible) envvars:
Perhaps these should be changed to the official AWS envvars.
Todo:
PopulatePageFromClusterLoadCluster