HPCC-XXXXX Add S3 direct API storage plane support - #21199
Conversation
e724e28 to
cc22aa6
Compare
896b057 to
24803cc
Compare
ghalliday
left a comment
ghalliday
left a comment
There was a problem hiding this comment.
@giftyedwin1 a few comments. The api copying all looks very useful. It may be worth comparing with the existing version since it may have changed since you first looked at it.
| // Use dlsym to find whichever exists | ||
| typedef IPropertyTree * (*fn2_t)(const char *, bool); | ||
| typedef IPropertyTree * (*fn1_t)(const char *); | ||
| static fn2_t fn2 = (fn2_t)dlsym(RTLD_DEFAULT, "_Z21getStoragePlaneConfigPKcb"); |
There was a problem hiding this comment.
This is probably better as a direct dependency to jlib. See https://github.com/hpcc-systems/HPCC-Platform/blob/master/common/remote/hooks/azure/CMakeLists.txt for a similar dependency in the azure api code.
Everything is versioned in sync, so not abi problems. getStoragePlaneConfig is preferred.
| auto & error = outcome.GetError(); | ||
| VStringBuffer msg("S3 copy: cannot stat %s/%s: %s - %s", srcBucket.str(), srcKey.str(), | ||
| error.GetExceptionName().c_str(), error.GetMessage().c_str()); | ||
| throw std::runtime_error(msg.str()); |
There was a problem hiding this comment.
We don't throw exceptions derived from std::exception in this code base (it predates the std library).
Instead we throw pointers to objects derived from IException. e.g.
throw makeStringException(code, text);
See handleRequestException in azureapiutils.cpp and the call from azurefile.cpp for similar translation.
| if (!multipartUpload->uploadPart(data, len)) | ||
| throw makeStringException(-1, "Failed to upload part to S3"); | ||
|
|
||
| pending.append(len, data); |
There was a problem hiding this comment.
This is going to involve cloning a significant amount of memory. How does it compare with the implementation in the current master?
I think that logic (which may have been rewritten since you first looked at it requires that the plane is configured with a block size >=5MB, so that each call to write can be sent as-is.
There was a problem hiding this comment.
Send-as-is did not work here: the engine writes ~1MB blocks (DEFAULT_BUFFER_SIZE, via createBufferedIOStream), so each multipart part is ~1MB and CompleteMultipartUpload fails with EntityTooSmall (S3 requires non-final parts ≥5MiB).
blockedSequentialIO doesn't help. Hence the write coalesces into ≥8MB parts, bounded to ~one part per writer rather than
the whole stream. thorbench1 passes at the default block size.
Is there a way to make the OUTPUT path emit ≥5MB blocks that I've missed?
237dc60 to
0cfa350
Compare
…3 storage plane hook Build on the existing S3 hook in master, adding the capabilities it lacks and fixing multipart upload sizing: - getCopyApiClient: server-side copy via CopyObject, with multipart UploadPartCopy for objects larger than 5GB (master returned nullptr). - directoryFiles: directory listing via ListObjectsV2 (was UNIMPLEMENTED). - Coalesce S3 multipart writes into >=5MB parts. S3FileWriteIO previously uploaded each engine write() (~1MB blocks) as its own multipart part, so non-final parts fell below S3's 5MB minimum and CompleteMultipartUpload failed with EntityTooSmall. Buffer writes and upload >=8MB parts, flushing the remainder as the final part on close() (last part may be <5MB, which S3 allows). Memory use is bounded to ~one part per writer. - Route the read, write and multipart upload paths through the same retryS3Op helper used by the new copy/listing clients, so retry/backoff handling is consistent across the hook (gatherMetadata stays inline as it treats NO_SUCH_KEY as a normal not-found result rather than a failure). Both API clients reuse the hook's existing S3 client cache and its handleRequestException / IException error handling. Adds queryPlaneName() to IStorageApiInfo so the copy client can resolve the source/target planes. Also adds storage-plane schema/Helm support for s3 buckets, S3 EKS Helm value examples, and an ECL regression test for the write paths.
0cfa350 to
d043b57
Compare
2 participants
Add S3 storage plane support via the file hook mechanism, enabling Thor and Roxie to read/write S3 objects transparently through the IFile/IFileIO interfaces.
Core implementation:
Platform changes:
Deployment:
Testing:
Type of change:
Checklist:
Smoketest:
Testing: