[ntuple] Add RPageSinkS3 for the basic S3 write path (Mode B)#22653
[ntuple] Add RPageSinkS3 for the basic S3 write path (Mode B)#22653JasMehta08 wants to merge 3 commits into
Conversation
JasMehta08
requested review from
bellenot,
jblomer and
silverweed
as code owners
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
Very nice! Some discussion points.
| fAnchor.fLenHeader = length; | ||
| } | ||
|
|
||
| ROOT::RNTupleLocator ROOT::Experimental::Internal::RPageSinkS3::CommitPageImpl(ColumnHandle_t columnHandle, |
There was a problem hiding this comment.
I think this is exactly the same code for file, DAOS, and S3. I suggest to make a first, preparatory commit that moves this logic to RPagePersistentSink::CommitPage() (the only place where it is used) and to remove CommitPageImpl() altogether.
There was a problem hiding this comment.
Should I create a separate PR or just a commit in this PR before the S3 sink commit
There was a problem hiding this comment.
I think it can be the same PR, just a different commit
There was a problem hiding this comment.
I was working on removing CommitPageImpl() altogether. I noticed a few things,
There are 5 places where it was present,
RPagePersistentSink, RPageSinkFile (A little different), RPageSinkDaos, RPageSinkS3 , RPageSinkTestLocator in ntuple_compat.cxx (a test) (different)
There was a problem hiding this comment.
- The file one differed, Its CommitPageImpl returns
WriteSealedPage(sealedPage, element->GetPackedSize(page.GetNElements()))instead ofCommitSealedPageImpl(columnHandle.fPhysicalId, sealedPage)like DAOS and S3. - I checked whether routing the new common
CommitPage()throughCommitSealedPageImpl()changes anything for File, and it shouldn't, File'sCommitSealedPageImplcomputes the same packed size ((nBits * nElements + 7) / 8, which is exactly whatGetPackedSize()returns) and calls the same WriteSealedPage. So the written blob and the returned locator are identical.
There was a problem hiding this comment.
- The RPageSinkTestLocator, is a bit tougher,
- RPageSinkTestLocator puts an unknown locator type by overriding
CommitPageImpland shadowing the (non-virtual)WriteSealedPage. WithCommitPageImplgone, the base would callRPageSinkFile::CommitSealedPageImpl → RPageSinkFile::WriteSealedPage(statically bound), so the test's shadow would silently stop being used. - To keep it working I thought of changing
RPageSinkFile::CommitSealedPageImplfrom final to override and changed the test to overrideCommitSealedPageImpldirectly. Let me know if you would prefer another approach to it.
| // Upload the anchor LAST: once it exists at the base URL, a reader can assume the whole ntuple | ||
| // is complete. Never upload it before all other objects are in place. | ||
| const auto anchorJson = fAnchor.ToJSON(); | ||
| PutObject(fBaseUrl, reinterpret_cast<const unsigned char *>(anchorJson.data()), anchorJson.size()); |
There was a problem hiding this comment.
One thing I forgot (not for this PR): we should checksum the anchor. E.g., using some canonical json normalization. Let's discuss this in the next meeting.
Test Results 21 files 21 suites 3d 2h 11m 6s ⏱️ Results for commit 621a5d3. ♻️ This comment has been updated with latest results. |
JasMehta08
force-pushed
the
ntuple-s3-sink
branch
from
55e94d8 to
5841fc3
Compare
JasMehta08
force-pushed
the
ntuple-s3-sink
branch
from
5841fc3 to
5f33a1b
Compare
|
We have some merge conflicts which need to be resolved first |
JasMehta08
force-pushed
the
ntuple-s3-sink
branch
from
5f33a1b to
8adb1e0
Compare
|
I have resolved the conflicts |
| { | ||
| // The hidden (attribute-set) ntuple is stored under a reserved "_clone" sub-prefix so its objects and | ||
| // anchor can never collide with the main ntuple's numeric object keys ($baseurl/0, $baseurl/1, ...). | ||
| std::string cloneBaseUrl = fBaseUrl + "/_clone/" + std::string(name); |
There was a problem hiding this comment.
We probably should make this a template, too, and add it to the anchor. Like fCloneTemplate = ${baseurl}/_clone/${name}. Can be a follow-up PR.
|
|
||
| if (ROOT::StartsWith(location, "ntpl+s3")) { | ||
| #ifdef R__ENABLE_S3 | ||
| return std::make_unique<ROOT::Experimental::Internal::RPageSinkS3>(ntupleName, location, options); |
There was a problem hiding this comment.
Shouldn't this need the (guarded) ROOT/RPageStorageS3.hxx include?
There was a problem hiding this comment.
It is included near the top of the file at line 30 as
#ifdef R__ENABLE_S3
#include <ROOT/RPageStorageS3.hxx>
#endif
JasMehta08
force-pushed
the
ntuple-s3-sink
branch
from
8adb1e0 to
621a5d3
Compare
|
I have made the changes according to the comments please have a look |
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
Thanks! For the Windows failures, the test should use gSystem's wrapper for setenv and unsetenv.
JasMehta08
force-pushed
the
ntuple-s3-sink
branch
from
621a5d3 to
fbb28cd
Compare
|
I have fixed the issue with |
3 participants
This Pull request:
(Is a part of the GSoC 2026 project
S3 Backend for RNTuple.)Adds
RPageSinkS3, the write path of the experimental S3 storage backend for RNTuple.Changes or fixes:
RPageSinkS3(Mode B full write path):InitImpl(header),CommitPageImpl/CommitSealedPageImpl(one S3 object per sealed page,kTypeObject64locator),StageClusterImpl,CommitClusterGroupImpl(page list), andCommitDatasetImpl(footer, then the JSON anchor written last for atomicity).Checklist: