morningman commented on code in PR #31716:
URL: https://github.com/apache/doris/pull/31716#discussion_r1522659915
##########
be/src/io/fs/hdfs_file_writer.cpp:
##########
@@ -96,53 +91,43 @@ Status HdfsFileWriter::appendv(const Slice* data, size_t
data_cnt) {
}
// Call this method when there is no more data to write.
-// FIXME(cyx): Does not seem to be an appropriate interface for file system?
Status HdfsFileWriter::finalize() {
- DCHECK(!_closed);
- if (_opened) {
- RETURN_IF_ERROR(close());
+ if (_closed) [[unlikely]] {
+ return Status::InternalError("finalize closed file: {}",
_path.native());
}
- return Status::OK();
+ // FIXME(plat1ko): `finalize` method should not be an operation which can
be blocked for a long time
+ return close();
}
-Status HdfsFileWriter::open() {
- if (_create_empty_file && !_opened) {
- RETURN_IF_ERROR(_open());
- _opened = true;
- }
- return Status::OK();
-}
-
-Status HdfsFileWriter::_open() {
- _path = convert_path(_path, _hdfs_fs->_fs_name);
- std::string hdfs_dir = _path.parent_path().string();
- int exists = hdfsExists(_hdfs_fs->_fs_handle->hdfs_fs, hdfs_dir.c_str());
+Result<FileWriterPtr> HdfsFileWriter::create(Path full_path, HdfsHandler*
handler,
+ const std::string& fs_name) {
+ auto path = convert_path(full_path, fs_name);
+ std::string hdfs_dir = path.parent_path().string();
Review Comment:
```suggestion
std::string hdfs_dir = path.parent_path().string();
```
Why calling `parent_path()` here? Please give an example in code comment
##########
be/src/agent/task_worker_pool.cpp:
##########
@@ -1034,10 +1035,16 @@ void report_tablet_callback(StorageEngine& engine,
const TMasterInfo& master_inf
}
request.__isset.storage_policy = true;
auto& resource_list = request.resource;
- for (auto [id, version] : get_storage_resource_ids()) {
+ for (auto [id_str, version] : get_storage_resource_ids()) {
auto& resource = resource_list.emplace_back();
- resource.__set_id(id);
- resource.__set_version(version);
+ int64_t id = -1;
+ if (auto [_, ec] = std::from_chars(id_str.data(), id_str.data() +
id_str.size(), id);
+ ec == std::errc {}) [[unlikely]] {
+ LOG(ERROR) << "invalid resource id format: " << id_str;
Review Comment:
```suggestion
LOG(WARNING) << "invalid resource id format: " << id_str;
```
##########
be/src/olap/rowset/segment_v2/inverted_index_compound_directory.cpp:
##########
@@ -512,11 +511,7 @@ void DorisCompoundDirectory::FSIndexOutput::close() {
int64_t DorisCompoundDirectory::FSIndexOutput::length() const {
CND_PRECONDITION(_writer != nullptr, "file is not open");
- int64_t ret;
- if (!_writer->fs()->file_size(_writer->path(), &ret).ok()) {
- return -1;
- }
- return ret;
+ return _writer->bytes_appended();
Review Comment:
The default value of `_bytes_appended` is 0,
but the origin logic may return -1, need to check if `-1` means something to
the caller.
##########
be/src/olap/rowset/segment_v2/inverted_index_compound_directory.cpp:
##########
@@ -399,8 +399,7 @@ void
DorisCompoundDirectory::FSIndexInput::readInternal(uint8_t* b, const int32_
void DorisCompoundDirectory::FSIndexOutput::init(const io::FileSystemSPtr&
fileSystem,
const char* path) {
- io::FileWriterOptions opts {.create_empty_file = false};
- Status status = fileSystem->create_file(path, &_writer, &opts);
+ Status status = fileSystem->create_file(path, &_writer);
Review Comment:
```suggestion
Status status = fs->create_file(path, &_writer);
```
##########
be/src/olap/rowset/vertical_beta_rowset_writer_helper.cpp:
##########
@@ -197,9 +197,7 @@ Status
VerticalBetaRowsetWriterHelper::_create_segment_writer(
return Status::Error<INIT_FAILED>("get fs failed");
}
io::FileWriterPtr file_writer;
- io::FileWriterOptions opts;
- opts.create_empty_file = false;
- Status st = fs->create_file(path, &file_writer, &opts);
+ Status st = fs->create_file(path, &file_writer);
Review Comment:
the default behavior of the file writer will create empty file.
But here we should not create empty file, so why remove
`opts.create_empty_file = false;`?
##########
be/src/runtime/snapshot_loader.cpp:
##########
@@ -102,19 +102,16 @@ Status SnapshotLoader::init(TStorageBackendType::type
type, const std::string& l
S3URI s3_uri(location);
RETURN_IF_ERROR(s3_uri.parse());
RETURN_IF_ERROR(S3ClientFactory::convert_properties_to_s3_conf(_prop,
s3_uri, &s3_conf));
- std::shared_ptr<io::S3FileSystem> fs;
- RETURN_IF_ERROR(io::S3FileSystem::create(std::move(s3_conf), "", &fs));
- _remote_fs = std::move(fs);
+ _remote_fs =
Review Comment:
Better to create a FsFactory class?
Maybe in next PR
--
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
