Add path info of replica in catalog #327
Conversation
Also fix a bug that when calling check_none_row_oriented_table, store is null, it cannot be used to create table. Instead, OLAPHeader can be used to get storage type information.
be/src/util/string_util.h
Outdated
| continue; | ||
| } | ||
|
|
||
| boost::hash_combine<std::string>(hash, part); |
There was a problem hiding this comment.
May be use the hash function in util/hash_util.hpp? it has been already optimized
There was a problem hiding this comment.
std::hash has already been used in other functions in string_util.h.
So just for unification.
be/src/util/string_util.h
Outdated
| std::size_t hash = std::hash<std::string>()(identifier); | ||
| std::vector<std::string> path_parts; | ||
| boost::split(path_parts, path, boost::is_any_of("/")); | ||
| for (std::string part : path_parts) { |
There was a problem hiding this comment.
You should use reference for part, And you can use auto
| for (std::string part : path_parts) { | |
| for (auto& part : path_parts) { |
be/src/olap/options.h
Outdated
| StorePath(const std::string& path_, int64_t capacity_bytes_) | ||
| : path(path_), capacity_bytes(capacity_bytes_) { } | ||
| std::string path; | ||
| int64_t path_hash; |
There was a problem hiding this comment.
Why add path_hash here.
StorePath is what user can define, not store's status
There was a problem hiding this comment.
forgot to remove it
| LOG(INFO) << "Master FE is changed or restarted. report tablet and disk info immediately"; | ||
| std::unique_lock<std::mutex> lk(OLAPEngine::get_instance()->report_mtx); | ||
| OLAPEngine::get_instance()->report_cv.notify_all(); | ||
| } |
There was a problem hiding this comment.
notify_allneedn't to hold mutex first.report_cv.notify_all()should encapsulate in a function- you can use
_olap_engineinstead ofOLAPEngine::get_instance()
| } else { | ||
| // when Master FE restarted, host and port remains the same, but epoch will be increased. | ||
| if (master_info.epoch > _epoch) { | ||
| _epoch = master_info.epoch; |
There was a problem hiding this comment.
this function may be called concurrently. and changing _epoch and testing _epoch without any protect would lead to strange result
There was a problem hiding this comment.
Add a mutex to protect _heartbeat() function
be/src/olap/olap_engine.cpp
Outdated
| if (_is_drop_tables) { | ||
| disk_broken_cv.notify_all(); | ||
| std::unique_lock<std::mutex> lk(report_mtx); | ||
| report_cv.notify_all(); |
There was a problem hiding this comment.
notify_all doesn't need lock held
be/src/util/string_util.h
Outdated
| } | ||
| }; | ||
|
|
||
| struct PathHash { |
There was a problem hiding this comment.
why do you define a struct? A function is better.
There was a problem hiding this comment.
I don't know why other 'functions' in string_util are defined as struct. Just for unification.
There was a problem hiding this comment.
em...
Other struct can be used for std::map or std::unordered_map. However your struct can't be used as a comparator or a hasher for these container.
So, you'd better to define a function here.
There was a problem hiding this comment.
I got it. Fixed.
4 participants
ISSUE #315
Also fix a bug that when calling check_none_row_oriented_table,
store is null, it cannot be used to create table.
Instead, OLAPHeader can be used to get storage type information.