github-actions[bot] commented on code in PR #65504:
URL: https://github.com/apache/doris/pull/65504#discussion_r3819932951
##########
be/src/io/fs/s3_file_writer.cpp:
##########
@@ -399,7 +399,9 @@ void S3FileWriter::_upload_one_part(int part_num,
UploadFileBuffer& buf) {
s3_bytes_written_total << buf.get_size();
ObjStorageCompletedPart completed_part {
- part_num, resp.etag.has_value() ? std::move(resp.etag.value()) :
""};
+ .part_num = part_num,
+ .etag = resp.etag.has_value() ? std::move(resp.etag.value()) : "",
+ .checksum_crc32c = std::move(resp.checksum_crc32c)};
Review Comment:
[P1] Preserve CRC32C through deferred Hive MPU completion
S3 Express is admitted to the Hive committer path, but this checksum is not
represented by `TS3MPUPendingUpload`; only part number and ETag reach FE. The
map overload then creates checksum-less `UploadPartResult`s, and
`S3ExpressObjStorage.completeMultipartUpload()` rejects them, so every Hive
write fails late after uploading its parts. Please either reject S3 Express in
`hive_multipart_protocol_supported()` before creating the MPU, as the
documented support boundary implies, or extend the deferred protocol to carry
each part's CRC32C into FE completion.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1496,7 +1498,8 @@ void
MetaServiceImpl::alter_storage_vault(google::protobuf::RpcController* contr
}
if (use_credential_provider(obj)) {
- if (!obj.has_provider() || obj.provider() !=
ObjectStoreInfoPB::S3) {
+ if (!obj.has_provider() || (obj.provider() !=
ObjectStoreInfoPB::S3 &&
+ obj.provider() !=
ObjectStoreInfoPB::S3EXPRESS)) {
Review Comment:
[P1] Enforce S3 Express invariants at the persistence boundary
The new FE checks `use_path_style=false` and non-anonymous credentials, but
MetaService accepts this provider without repeating those invariants.
`ADD_BUILT_IN_VAULT` has the same gap, and partial ALTER can merge
`use_path_style=true` (or a role change paired with ANONYMOUS) into an existing
Express object without validating the result. An older/rollback FE or direct
MetaService caller can therefore persist a vault that the recycler and BE
reject, making it unusable. Please centralize validation for `ADD_S3_VAULT`,
`ADD_BUILT_IN_VAULT`, and the fully merged `ALTER_S3_VAULT` result, enforcing
virtual-hosted access and non-anonymous credentials.
##########
common/cpp/obj-client/s3_express_obj_storage_client.cpp:
##########
@@ -0,0 +1,159 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "s3_express_obj_storage_client.h"
+
+#include <aws/s3/model/ChecksumAlgorithm.h>
+#include <aws/s3/model/HeadBucketRequest.h>
+
+#include <algorithm>
+#include <string>
+#include <string_view>
+
+namespace doris {
+namespace {
+
+std::string directory_bucket_list_prefix(std::string_view logical_prefix) {
+ if (logical_prefix.empty() || logical_prefix.ends_with('/')) {
+ return std::string(logical_prefix);
+ }
+
+ auto separator = logical_prefix.rfind('/');
+ return separator == std::string_view::npos
+ ? std::string()
+ : std::string(logical_prefix.substr(0, separator + 1));
+}
+
+template <typename Request>
+void set_crc32c_checksum(Request& request, Aws::IOStream& stream) {
+ Aws::Utils::ByteBuffer
crc32c(Aws::Utils::HashingUtils::CalculateCRC32C(stream));
+ request.SetChecksumAlgorithm(Aws::S3::Model::ChecksumAlgorithm::CRC32C);
+ request.SetChecksumCRC32C(Aws::Utils::HashingUtils::Base64Encode(crc32c));
+}
+
+} // namespace
+
+ObjStorageListPageResult S3ExpressObjStorageClient::list_objects_page(
+ const ObjStoragePath& opts, std::string_view continuation_token) {
+ const std::string logical_prefix = opts.prefix.empty() ? opts.key :
opts.prefix;
+ const std::string list_prefix =
directory_bucket_list_prefix(logical_prefix);
+
+ auto list_opts = opts;
+ list_opts.key.clear();
+ list_opts.prefix = list_prefix;
+ auto page = S3ObjStorageClient::list_objects_page(list_opts,
continuation_token);
+ if (!page.resp.ok() || logical_prefix == list_prefix) {
+ return page;
+ }
+
+ std::erase_if(page.objects, [&logical_prefix](const ObjectMeta& object) {
+ return !object.key.starts_with(logical_prefix);
+ });
+ return page;
+}
+
+ObjStorageResponse S3ExpressObjStorageClient::head_bucket(const std::string&
bucket) {
+ Aws::S3::Model::HeadBucketRequest request;
+ request.SetBucket(bucket);
+ auto outcome = standard_auth_client_->HeadBucket(request);
+ if (outcome.IsSuccess()) {
+ return ObjStorageResponse::OK();
+ }
+
record_object_request_failed(static_cast<int>(outcome.GetError().GetResponseCode()));
+ return {
+ .status = s3fs_error(outcome.GetError(),
+ fmt::format("failed to head bucket: {}",
bucket)),
+ .http_code =
static_cast<int>(outcome.GetError().GetResponseCode()),
+ .request_id = outcome.GetError().GetRequestId(),
+ };
+}
+
+std::string S3ExpressObjStorageClient::generate_presigned_url(const
ObjStoragePath& opts,
+ int64_t
expiration_secs) {
+ // Session credentials expire after five minutes. Use the standard SigV4
client so a
+ // presigned URL remains valid for the expiration requested by the caller
(subject to the
+ // lifetime of the configured IAM/STS credentials).
+ return standard_auth_client_->GeneratePresignedUrl(
+ opts.bucket, opts.key, Aws::Http::HttpMethod::HTTP_GET,
expiration_secs);
+}
+
+ObjStorageResponse S3ExpressObjStorageClient::get_lifecycle(const std::string&
/*bucket*/,
+ int64_t*
expiration_days) {
+ // Directory buckets do not support the noncurrent-version lifecycle rule
checked by
+ // InstanceChecker.
+ *expiration_days = INT64_MAX;
Review Comment:
[P1] Do not suppress lifecycle protection for directory buckets
Returning `INT64_MAX` makes an all-Express instance take
`Checker::do_inspect()`'s 'no S3 bucket' branch; in a mixed instance the
Express lifecycle rule is still omitted from the minimum. [Directory buckets
support current-object
Expiration](https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketLifecycleConfiguration.html),
and without versioning such a rule permanently deletes committed Doris data;
the new guide explicitly warns that Doris currently will not detect it. Please
inspect `GetBucketLifecycleConfiguration` through the
standard-auth/control-plane client and reject or alarm on overlapping
current-object expiration rules, or fail/reject Express vaults when that safety
check cannot be performed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
