morningman commented on code in PR #20926:
URL: https://github.com/apache/doris/pull/20926#discussion_r1232351246


##########
be/src/io/fs/benchmark/s3_benchmark.hpp:
##########
@@ -52,4 +55,164 @@ class S3ReadBenchmark : public BaseBenchmark {
     size_t _bytes_read = 0;
 };
 
+class S3SizeBenchmark : public BaseBenchmark {

Review Comment:
   This is meaningless. Because the method `size()` is just return the `_size` 
field.
   No action to do with S3.



##########
be/src/io/fs/benchmark/s3_benchmark.hpp:
##########
@@ -52,4 +55,164 @@ class S3ReadBenchmark : public BaseBenchmark {
     size_t _bytes_read = 0;
 };
 
+class S3SizeBenchmark : public BaseBenchmark {
+public:
+    S3SizeBenchmark(int iterations, const std::map<std::string, std::string>& 
conf_map)
+            : BaseBenchmark("S3SizeBenchmark", iterations, conf_map) {}
+    virtual ~S3SizeBenchmark() = default;
+
+    Status init() override {
+        bm_log("begin to init {}", _name);
+        std::string file_path = _conf_map["file"];
+        io::FileReaderOptions reader_opts = 
FileFactory::get_reader_options(nullptr);
+        RETURN_IF_ERROR(
+                FileFactory::create_s3_reader(_conf_map, file_path, &_fs, 
&_reader, reader_opts));
+        bm_log("finish to init {}", _name);
+        return Status::OK();
+    }
+
+    Status run() override {
+        size_t i = _reader->size();
+        bm_log("size result is: {}", i);
+        return Status::OK();
+    }
+
+private:
+    std::shared_ptr<io::FileSystem> _fs;
+    io::FileReaderSPtr _reader;
+};
+
+class S3ListBenchmark : public BaseBenchmark {
+public:
+    S3ListBenchmark(int iterations, const std::map<std::string, std::string>& 
conf_map)
+            : BaseBenchmark("S3ListBenchmark", iterations, conf_map) {}
+    virtual ~S3ListBenchmark() = default;
+
+    Status init() override {
+        bm_log("begin to init {}", _name);
+        S3URI s3_uri(_conf_map["file"]);
+        RETURN_IF_ERROR(s3_uri.parse());
+        RETURN_IF_ERROR(
+                S3ClientFactory::convert_properties_to_s3_conf(_conf_map, 
s3_uri, &_s3_conf));
+        RETURN_IF_ERROR(io::S3FileSystem::create(std::move(_s3_conf), "", 
&_fs));
+        bm_log("finish to init {}", _name);
+        return Status::OK();
+    }
+
+    Status run() override {
+        std::vector<FileInfo> files;
+        bool exists = true;
+        _fs->list(_conf_map["file"], true, &files, &exists);
+        bm_log("list result is: {}", files.size());
+        return Status::OK();
+    }
+
+private:
+    doris::S3Conf _s3_conf;
+    std::shared_ptr<io::S3FileSystem> _fs;
+};
+
+class S3OpenBenchmark : public BaseBenchmark {
+public:
+    S3OpenBenchmark(int iterations, const std::map<std::string, std::string>& 
conf_map)
+            : BaseBenchmark("S3OpenBenchmark", iterations, conf_map) {}
+    virtual ~S3OpenBenchmark() = default;
+
+    Status init() override {
+        bm_log("begin to init {}", _name);
+        S3URI s3_uri(_conf_map["file"]);
+        RETURN_IF_ERROR(s3_uri.parse());
+        RETURN_IF_ERROR(
+                S3ClientFactory::convert_properties_to_s3_conf(_conf_map, 
s3_uri, &_s3_conf));
+        RETURN_IF_ERROR(io::S3FileSystem::create(std::move(_s3_conf), "", 
&_fs));
+
+        std::string file_path = _conf_map["file"];
+        io::FileReaderOptions reader_opts = 
FileFactory::get_reader_options(nullptr);
+        RETURN_IF_ERROR(FileFactory::create_s3_reader(
+                _conf_map, file_path, 
reinterpret_cast<std::shared_ptr<io::FileSystem>*>(&_fs),
+                &_reader, reader_opts));
+        bm_log("finish to init {}", _name);
+        return Status::OK();
+    }
+
+    Status run() override { return _fs->open_file(_conf_map["file"], 
&_reader); }

Review Comment:
   Missing `const FileReaderOptions& reader_options` in `open_file`'s parameter 
list?



##########
be/src/io/fs/benchmark/s3_benchmark.hpp:
##########
@@ -52,4 +55,164 @@ class S3ReadBenchmark : public BaseBenchmark {
     size_t _bytes_read = 0;
 };
 
+class S3SizeBenchmark : public BaseBenchmark {

Review Comment:
   You need to call `s3_file_system`'s `file_size_impl()`



##########
be/src/io/fs/benchmark/s3_benchmark.hpp:
##########
@@ -52,4 +55,164 @@ class S3ReadBenchmark : public BaseBenchmark {
     size_t _bytes_read = 0;
 };
 
+class S3SizeBenchmark : public BaseBenchmark {
+public:
+    S3SizeBenchmark(int iterations, const std::map<std::string, std::string>& 
conf_map)
+            : BaseBenchmark("S3SizeBenchmark", iterations, conf_map) {}
+    virtual ~S3SizeBenchmark() = default;
+
+    Status init() override {
+        bm_log("begin to init {}", _name);
+        std::string file_path = _conf_map["file"];
+        io::FileReaderOptions reader_opts = 
FileFactory::get_reader_options(nullptr);
+        RETURN_IF_ERROR(
+                FileFactory::create_s3_reader(_conf_map, file_path, &_fs, 
&_reader, reader_opts));
+        bm_log("finish to init {}", _name);
+        return Status::OK();
+    }
+
+    Status run() override {
+        size_t i = _reader->size();
+        bm_log("size result is: {}", i);
+        return Status::OK();
+    }
+
+private:
+    std::shared_ptr<io::FileSystem> _fs;
+    io::FileReaderSPtr _reader;
+};
+
+class S3ListBenchmark : public BaseBenchmark {
+public:
+    S3ListBenchmark(int iterations, const std::map<std::string, std::string>& 
conf_map)
+            : BaseBenchmark("S3ListBenchmark", iterations, conf_map) {}
+    virtual ~S3ListBenchmark() = default;
+
+    Status init() override {
+        bm_log("begin to init {}", _name);
+        S3URI s3_uri(_conf_map["file"]);
+        RETURN_IF_ERROR(s3_uri.parse());
+        RETURN_IF_ERROR(
+                S3ClientFactory::convert_properties_to_s3_conf(_conf_map, 
s3_uri, &_s3_conf));
+        RETURN_IF_ERROR(io::S3FileSystem::create(std::move(_s3_conf), "", 
&_fs));
+        bm_log("finish to init {}", _name);
+        return Status::OK();
+    }
+
+    Status run() override {
+        std::vector<FileInfo> files;
+        bool exists = true;
+        _fs->list(_conf_map["file"], true, &files, &exists);
+        bm_log("list result is: {}", files.size());

Review Comment:
   log here is too much



##########
be/src/runtime/exec_env.h:
##########
@@ -182,6 +182,9 @@ class ExecEnv {
         this->_stream_load_executor = stream_load_executor;
     }
 
+    // Threadpool used to prefetch remote file for buffered reader
+    std::unique_ptr<ThreadPool> _buffered_reader_prefetch_thread_pool;

Review Comment:
   Should not just move it to public. Use get and set method.



-- 
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

Reply via email to