gavinchou commented on code in PR #35307: URL: https://github.com/apache/doris/pull/35307#discussion_r1616634603
########## be/src/io/fs/obj_storage_client.h: ########## @@ -0,0 +1,83 @@ +// 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. + +#pragma once + +#include <optional> + +#include "io/fs/file_system.h" +#include "io/fs/path.h" +namespace doris { +class Status; +namespace io { + +enum class ObjStorageType : uint8_t { + AZURE = 0, + BOS, + COS, + OSS, + OBS, + GCP, + S3, +}; + +struct ObjectStoragePathOptions { + Path path; + std::string bucket; // blob container in azure + std::string key; // blob name + std::string prefix; // for batch delete and recursive delete + std::optional<std::string> upload_id; // only used for S3 upload +}; + +struct ObjectCompleteMultiParts {}; + +struct ObjectStorageResponse { + Status status; // Azure的异常里面http的信息等都有，如果是S3的话则用s3fs_error + std::optional<std::string> upload_id; + std::optional<std::string> etag; +}; + +struct ObjectStorageHeadResponse { + Status status; + long long file_size {0}; +}; + +// wrapper class owned by concret fs +class ObjStorageClient { Review Comment: Add detailed comments for each function/interface, describing how will the function act and what params should be passed-in. ########## be/src/io/fs/obj_storage_client.h: ########## @@ -0,0 +1,83 @@ +// 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. + +#pragma once + +#include <optional> + +#include "io/fs/file_system.h" +#include "io/fs/path.h" +namespace doris { +class Status; +namespace io { + +enum class ObjStorageType : uint8_t { + AZURE = 0, + BOS, + COS, + OSS, + OBS, + GCP, + S3, Review Comment: Use AWS instead S3 or just AWS_S3? The name S3 is abuse-used before this commit. Also add comment that the names are in lexico order. ########## be/src/io/fs/s3_file_reader.cpp: ########## @@ -108,22 +109,20 @@ Status S3FileReader::read_at_impl(size_t offset, Slice result, size_t* bytes_rea return Status::OK(); } - Aws::S3::Model::GetObjectRequest request; - request.WithBucket(_bucket).WithKey(_key); - request.SetRange(fmt::format("bytes={}-{}", offset, offset + bytes_req - 1)); - request.SetResponseStreamFactory(AwsWriteableStreamFactory(to, bytes_req)); - auto client = _client->get(); if (!client) { return Status::InternalError("init s3 client error"); } - SCOPED_BVAR_LATENCY(s3_bvar::s3_get_latency); - auto outcome = client->GetObject(request); - if (!outcome.IsSuccess()) { - return s3fs_error(outcome.GetError(), + auto resp = client->GetObject( + { Review Comment: Strange indention. ########## be/src/io/fs/obj_storage_client.h: ########## @@ -0,0 +1,83 @@ +// 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. + +#pragma once + +#include <optional> + +#include "io/fs/file_system.h" +#include "io/fs/path.h" +namespace doris { +class Status; +namespace io { + +enum class ObjStorageType : uint8_t { + AZURE = 0, + BOS, + COS, + OSS, + OBS, + GCP, + S3, +}; + +struct ObjectStoragePathOptions { + Path path; + std::string bucket; // blob container in azure + std::string key; // blob name + std::string prefix; // for batch delete and recursive delete + std::optional<std::string> upload_id; // only used for S3 upload +}; + +struct ObjectCompleteMultiParts {}; + +struct ObjectStorageResponse { + Status status; // Azure的异常里面http的信息等都有，如果是S3的话则用s3fs_error + std::optional<std::string> upload_id; + std::optional<std::string> etag; +}; + +struct ObjectStorageHeadResponse { + Status status; + long long file_size {0}; +}; + +// wrapper class owned by concret fs +class ObjStorageClient { +public: + virtual ~ObjStorageClient() = default; + virtual ObjectStorageResponse CreateMultiPartUpload(const ObjectStoragePathOptions& opts) = 0; Review Comment: Naming style should be `lower_case_under_score` not `UpperCaseCamel` ########## be/src/io/fs/obj_storage_client.h: ########## @@ -0,0 +1,83 @@ +// 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. + +#pragma once + +#include <optional> + +#include "io/fs/file_system.h" +#include "io/fs/path.h" +namespace doris { +class Status; +namespace io { + +enum class ObjStorageType : uint8_t { + AZURE = 0, + BOS, + COS, + OSS, + OBS, + GCP, + S3, +}; + +struct ObjectStoragePathOptions { + Path path; + std::string bucket; // blob container in azure + std::string key; // blob name + std::string prefix; // for batch delete and recursive delete + std::optional<std::string> upload_id; // only used for S3 upload +}; + +struct ObjectCompleteMultiParts {}; + +struct ObjectStorageResponse { + Status status; // Azure的异常里面http的信息等都有，如果是S3的话则用s3fs_error + std::optional<std::string> upload_id; + std::optional<std::string> etag; +}; + +struct ObjectStorageHeadResponse { + Status status; + long long file_size {0}; +}; + +// wrapper class owned by concret fs +class ObjStorageClient { +public: + virtual ~ObjStorageClient() = default; + virtual ObjectStorageResponse CreateMultiPartUpload(const ObjectStoragePathOptions& opts) = 0; + virtual ObjectStorageResponse PutObject(const ObjectStoragePathOptions& opts, + std::string_view stream) = 0; + virtual ObjectStorageResponse UploadPart(const ObjectStoragePathOptions& opts, + std::string_view stream, int partNum) = 0; + virtual ObjectStorageResponse CompleteMultipartUpload( + const ObjectStoragePathOptions& opts, + const ObjectCompleteMultiParts& completed_parts) = 0; + virtual ObjectStorageHeadResponse HeadObject(const ObjectStoragePathOptions& opts) = 0; + virtual ObjectStorageResponse GetObject(const ObjectStoragePathOptions& opts, void* buffer, + size_t offset, size_t bytes_read, + size_t* size_return) = 0; + virtual ObjectStorageResponse ListObjects(const ObjectStoragePathOptions& opts, + std::vector<FileInfo>* files) = 0; + virtual ObjectStorageResponse DeleteObjects(const ObjectStoragePathOptions& opts, + std::vector<std::string> objs) = 0; + virtual ObjectStorageResponse DeleteObject(const ObjectStoragePathOptions& opts) = 0; + virtual ObjectStorageResponse RecursiveDelete(const ObjectStoragePathOptions& opts) = 0; Review Comment: Naming `RecursiveDelete` -> `DeleteObjectsRecursively` -- 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: commits-unsubscr...@doris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org For additional commands, e-mail: commits-h...@doris.apache.org
