Skip to content

Conversation

@kangpinghuang
Copy link
Contributor

No description provided.

}
}

bool StorageEngine::check_path_in_unused_rowsets(const string& path) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some comment to describe the path strategy

// gc thread should get tablet include deleted tablet
// or it will delete rowset file before tablet is garbage collected
RowsetId rowset_id = -1;
bool is_rowset_file = _tablet_manager->get_rowset_id_from_path(path, &rowset_id);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_rowset_id_from_path should in rowset class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, because this is a path pattern. And more here you can not get a rowset to get the rowsetid

std::atomic<bool> _is_bad; // if this tablet is broken, set to true. default is false
std::atomic<int64_t> _last_compaction_failure_time; // timestamp of last compaction failure
RowsetId _start_rowset_id;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

start_rowset_id is useless?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will remove it.

}
for (auto& inc_version_rowset : _inc_rs_version_map) {
bool ret = inc_version_rowset.second->check_path(path_to_check);
if (ret) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe only check rowset id not the full path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will add check_rowset_id to do this

if (is_rowset_file) {
TabletSharedPtr tablet = _tablet_manager->get_tablet(tablet_id, schema_hash, true);
if (tablet != nullptr) {
bool valid = tablet->check_path(path);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should sleep in path scan method to prevent io 100%

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I have sleep in line 944

_process_garbage_path(path);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sleep every 1000 path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

every path_gc_check_step paths, I will sleep 10ms, it is a config.

Copy link
Contributor

@yiguolei yiguolei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

_gc_mutex.lock();
for (auto& _unused_rowset_pair : _unused_rowsets) {
if (_unused_rowset_pair.second->check_path(path)) {
LOG(INFO) << "path is found in unused rowsets, path:" << path;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not suggest to print log in lock

}

bool Tablet::check_rowset_id(RowsetId rowset_id) {
bool ret = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_rs_version_map will be modified when you call this function.
A readlock is necessary.

bool Tablet::check_rowset_id(RowsetId rowset_id) {
bool ret = false;
for (auto& version_rowset : _rs_version_map) {
ret = version_rowset.second->rowset_id() == rowset_id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (version_rowset.second->rowset_id() == rowset_id) {
return true;
}

}

for (auto& inc_version_rowset : _inc_rs_version_map) {
ret = inc_version_rowset.second->rowset_id() == rowset_id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (inc_version_rowset.second->rowset_id() == rowset_id) {
return true;
}

@chaoyli chaoyli merged commit 0adf413 into apache:be_refactor Jun 25, 2019
luwei16 pushed a commit to luwei16/Doris that referenced this pull request Apr 7, 2023
swjtu-zhanglei added a commit to swjtu-zhanglei/incubator-doris that referenced this pull request Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants