xiaokang commented on code in PR #35459:
URL: https://github.com/apache/doris/pull/35459#discussion_r1634342972
##########
be/src/olap/task/index_builder.cpp:
##########
@@ -251,7 +251,7 @@ Status
IndexBuilder::handle_single_rowset(RowsetMetaSharedPtr output_rowset_meta
const auto& output_rs_tablet_schema =
output_rowset_meta->tablet_schema();
if (output_rs_tablet_schema->get_inverted_index_storage_format() !=
InvertedIndexStorageFormatPB::V1) {
- const auto& fs = io::global_local_filesystem();
+ const auto& fs = output_rowset_meta->fs();
Review Comment:
Is this a bug fix here? Yf yes, add a testcase.
##########
be/src/olap/compaction.cpp:
##########
@@ -645,20 +641,14 @@ Status Compaction::do_inverted_index_compaction() {
auto inverted_index_file_writer =
std::make_unique<InvertedIndexFileWriter>(
ctx.fs(), index_path_prefix, ctx.rowset_id.to_string(), i,
_cur_tablet_schema->get_inverted_index_storage_format());
- RETURN_NOT_OK_STATUS_WITH_WARN(
-
inverted_index_file_writer->initialize(index_not_need_to_compact),
- "failed to initialize inverted_index_file_writer for " +
- inverted_index_file_writer->get_index_file_path());
+
RETURN_IF_ERROR(inverted_index_file_writer->initialize(index_not_need_to_compact));
inverted_index_file_writers[i] =
std::move(inverted_index_file_writer);
} else if (st.is<ErrorCode::INVERTED_INDEX_FILE_NOT_FOUND>()) {
auto inverted_index_file_writer =
std::make_unique<InvertedIndexFileWriter>(
ctx.fs(), index_path_prefix, ctx.rowset_id.to_string(), i,
_cur_tablet_schema->get_inverted_index_storage_format());
inverted_index_file_writers[i] =
std::move(inverted_index_file_writer);
} else {
Review Comment:
error log missed
##########
be/src/olap/compaction.cpp:
##########
@@ -611,16 +611,12 @@ Status Compaction::do_inverted_index_compaction() {
auto seg_path = DORIS_TRY(rowset->segment_path(seg_id));
auto inverted_index_file_reader =
std::make_unique<InvertedIndexFileReader>(
- fs, std::string
{InvertedIndexDescriptor::get_index_path_prefix(seg_path)},
+ fs, std::string
{InvertedIndexDescriptor::get_index_file_path_prefix(seg_path)},
_cur_tablet_schema->get_inverted_index_storage_format());
bool open_idx_file_cache = false;
auto st =
inverted_index_file_reader->init(config::inverted_index_read_buffer_size,
open_idx_file_cache);
if (!st.ok()) {
Review Comment:
error log missed.
##########
be/src/olap/rowset/segment_v2/inverted_index_file_writer.cpp:
##########
@@ -125,22 +113,24 @@ Status InvertedIndexFileWriter::close() {
if (_indices_dirs.empty()) {
return Status::OK();
}
- try {
- if (_storage_format == InvertedIndexStorageFormatPB::V1) {
+ if (_storage_format == InvertedIndexStorageFormatPB::V1) {
+ try {
+ _file_size = write_v1();
for (const auto& entry : _indices_dirs) {
const auto& dir = entry.second;
- auto* cfsWriter = _CLNEW DorisCompoundFileWriter(dir.get());
- // write compound file
- _file_size += cfsWriter->writeCompoundFile();
// delete index path, which contains separated inverted index
files
if (std::strcmp(dir->getObjectName(), "DorisFSDirectory") ==
0) {
auto* compound_dir =
static_cast<DorisFSDirectory*>(dir.get());
compound_dir->deleteDirectory();
}
- _CLDELETE(cfsWriter)
}
- } else {
- _file_size = write();
+ } catch (CLuceneError& err) {
+ return Status::Error<ErrorCode::INVERTED_INDEX_CLUCENE_ERROR>(
+ "CLuceneError occur when close, error msg: {}",
err.what());
+ }
+ } else {
+ try {
+ _file_size = write_v2();
for (const auto& entry : _indices_dirs) {
Review Comment:
We can call write_v1() or write_v2() in if else and left other code the same
to remove duplicate code.
##########
be/src/olap/rowset/segment_v2/inverted_index_fs_directory.cpp:
##########
@@ -341,36 +340,22 @@ DorisFSDirectory::DorisFSDirectory() {
this->lockFactory = nullptr;
}
-void DorisFSDirectory::init(const io::FileSystemSPtr& _fs, const char* _path,
- bool use_compound_file_writer,
lucene::store::LockFactory* lock_factory,
- const io::FileSystemSPtr& cfs, const char*
cfs_path) {
- fs = _fs;
- directory = _path;
- useCompoundFileWriter = use_compound_file_writer;
-
- if (cfs == nullptr) {
- compound_fs = fs;
- } else {
- compound_fs = cfs;
- }
- if (cfs_path != nullptr) {
- cfs_directory = cfs_path;
- } else {
- cfs_directory = _path;
- }
-
+void DorisFSDirectory::init(const io::FileSystemSPtr& fs, const char* path,
+ lucene::store::LockFactory* lock_factory) {
+ _fs = fs;
+ directory = path;
if (lock_factory == nullptr) {
lock_factory = _CLNEW lucene::store::NoLockFactory();
}
lucene::store::Directory::setLockFactory(lock_factory);
// It's fail checking directory existence in S3.
- if (fs->type() == io::FileSystemType::S3) {
+ if (_fs->type() == io::FileSystemType::S3) {
return;
Review Comment:
Return here may skip other code logic. It's better to suround file exist
check in if (_fs->type() != io::FileSystemType::S3).
##########
be/src/olap/rowset/segment_v2/inverted_index_writer.cpp:
##########
@@ -537,8 +537,6 @@ class InvertedIndexColumnWriterImpl : public
InvertedIndexColumnWriter {
return 0;
}
- int64_t file_size() const override { return _dir->getCompoundFileSize(); }
Review Comment:
Is file_size() usefull if we want to add index size in the future?
##########
be/src/olap/task/index_builder.cpp:
##########
@@ -283,12 +283,10 @@ Status
IndexBuilder::handle_single_rowset(RowsetMetaSharedPtr output_rowset_meta
std::move(inverted_index_file_writer));
}
for (auto&& [seg_id, inverted_index_writer] :
_inverted_index_file_writers) {
- LOG(INFO) << "close inverted index file "
- << inverted_index_writer->get_index_file_path();
auto st = inverted_index_writer->close();
if (!st.ok()) {
- LOG(ERROR) << "close inverted index file "
- << inverted_index_writer->get_index_file_path()
<< " error:" << st;
+ LOG(ERROR) << "close inverted_index_writer error:" << st;
Review Comment:
miss file path info in log
--
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]
