You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2021/04/14 09:38:22 UTC
[GitHub] [incubator-doris] weizuo93 opened a new pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
weizuo93 opened a new pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651
## Proposed changes
Support to switching cumulative compaction policy between`num_based` and `size_based` dynamically without restarting BE.
## Types of changes
What types of changes does your code introduce to Doris?
_Put an `x` in the boxes that apply_
- [ ] Bugfix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Documentation Update (if none of the other choices apply)
- [x] Code refactor (Modify the code structure, format the code, etc...)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg merged pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
yangzhg merged pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] acelyc111 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r613169880
##########
File path: be/src/olap/tablet.cpp
##########
@@ -714,7 +718,7 @@ bool Tablet::can_do_compaction() {
return true;
}
-const uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) const {
+const uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) {
Review comment:
```suggestion
uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) {
```
##########
File path: be/src/common/configbase.cpp
##########
@@ -338,7 +340,11 @@ bool init(const char* conf_file, bool fill_conf_map, bool must_exist, bool set_t
SET_FIELD(it.second, int32_t, fill_conf_map, set_to_default);
SET_FIELD(it.second, int64_t, fill_conf_map, set_to_default);
SET_FIELD(it.second, double, fill_conf_map, set_to_default);
- SET_FIELD(it.second, std::string, fill_conf_map, set_to_default);
+ {
+ // add lock to ensure thread safe
+ std::lock_guard<std::mutex> lock(mutable_string_config_lock);
+ SET_FIELD(it.second, std::string, fill_conf_map, set_to_default);
+ }
Review comment:
`init` is called in main when no other threads created, is it needed to use lock?
##########
File path: be/src/olap/olap_server.cpp
##########
@@ -205,29 +205,32 @@ void StorageEngine::_disk_stat_monitor_thread_callback() {
}
void StorageEngine::_check_cumulative_compaction_config() {
- std::string cumulative_compaction_type = config::cumulative_compaction_policy;
+ std::string cumulative_compaction_type = "";
Review comment:
Most of these configs are mutable, check them firstly here may be useless. A better way is to check them when any of them are mutated?
##########
File path: be/src/olap/tablet.h
##########
@@ -162,7 +162,7 @@ class Tablet : public BaseTablet {
// operation for compaction
bool can_do_compaction();
- const uint32_t calc_compaction_score(CompactionType compaction_type) const;
+ const uint32_t calc_compaction_score(CompactionType compaction_type);
Review comment:
```suggestion
uint32_t calc_compaction_score(CompactionType compaction_type);
```
##########
File path: be/src/olap/tablet.cpp
##########
@@ -88,6 +88,10 @@ OLAPStatus Tablet::_init_once_action() {
<< ", version_size=" << _tablet_meta->version_count();
// init cumulative compaction policy by type
+ if (_cumulative_compaction_type == "") {
Review comment:
Now _cumulative_compaction_policy will be created in _calc_cumulative_compaction_score, is it needed to create it here initially?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] morningman commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r616595505
##########
File path: be/src/olap/tablet.cpp
##########
@@ -725,7 +727,23 @@ const uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) con
}
}
-const uint32_t Tablet::_calc_cumulative_compaction_score() const {
+const uint32_t Tablet::_calc_cumulative_compaction_score() {
+#ifndef BE_TEST
+ std::string current_policy = "";
+ {
+ std::lock_guard<std::mutex> lock(*config::get_mutable_string_config_lock());
Review comment:
It's better to check the config at the beginning of each `_compaction_tasks_producer_callback` loop,
instead of check it for each tablet.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] weizuo93 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
weizuo93 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r613201246
##########
File path: be/src/olap/tablet.cpp
##########
@@ -88,6 +88,10 @@ OLAPStatus Tablet::_init_once_action() {
<< ", version_size=" << _tablet_meta->version_count();
// init cumulative compaction policy by type
+ if (_cumulative_compaction_type == "") {
Review comment:
> Now _cumulative_compaction_policy will be created in _calc_cumulative_compaction_score, is it needed to create it here initially?
I also konw that we don't need to execute`create_cumulative_compaction_policy()`here, but UT will have some problems if we remove it. Next, I will refacor UT.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] weizuo93 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
weizuo93 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r613203192
##########
File path: be/src/olap/tablet.cpp
##########
@@ -714,7 +718,7 @@ bool Tablet::can_do_compaction() {
return true;
}
-const uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) const {
+const uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) {
Review comment:
>
OK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] weizuo93 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
weizuo93 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r617410118
##########
File path: be/src/olap/tablet.cpp
##########
@@ -725,7 +727,23 @@ const uint32_t Tablet::calc_compaction_score(CompactionType compaction_type) con
}
}
-const uint32_t Tablet::_calc_cumulative_compaction_score() const {
+const uint32_t Tablet::_calc_cumulative_compaction_score() {
+#ifndef BE_TEST
+ std::string current_policy = "";
+ {
+ std::lock_guard<std::mutex> lock(*config::get_mutable_string_config_lock());
Review comment:
> It's better to check the config at the beginning of each `_compaction_tasks_producer_callback` loop,
> instead of check it for each tablet.
Thanks, it sounds reasonable.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] weizuo93 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
weizuo93 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r613190823
##########
File path: be/src/common/configbase.cpp
##########
@@ -338,7 +340,11 @@ bool init(const char* conf_file, bool fill_conf_map, bool must_exist, bool set_t
SET_FIELD(it.second, int32_t, fill_conf_map, set_to_default);
SET_FIELD(it.second, int64_t, fill_conf_map, set_to_default);
SET_FIELD(it.second, double, fill_conf_map, set_to_default);
- SET_FIELD(it.second, std::string, fill_conf_map, set_to_default);
+ {
+ // add lock to ensure thread safe
+ std::lock_guard<std::mutex> lock(mutable_string_config_lock);
+ SET_FIELD(it.second, std::string, fill_conf_map, set_to_default);
+ }
Review comment:
> `init` is called in main when no other threads created, is it needed to use lock?
Thanks, you are right.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] weizuo93 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
weizuo93 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r613203394
##########
File path: be/src/olap/tablet.h
##########
@@ -162,7 +162,7 @@ class Tablet : public BaseTablet {
// operation for compaction
bool can_do_compaction();
- const uint32_t calc_compaction_score(CompactionType compaction_type) const;
+ const uint32_t calc_compaction_score(CompactionType compaction_type);
Review comment:
>
OK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] weizuo93 commented on a change in pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
weizuo93 commented on a change in pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651#discussion_r613202913
##########
File path: be/src/olap/olap_server.cpp
##########
@@ -205,29 +205,32 @@ void StorageEngine::_disk_stat_monitor_thread_callback() {
}
void StorageEngine::_check_cumulative_compaction_config() {
- std::string cumulative_compaction_type = config::cumulative_compaction_policy;
+ std::string cumulative_compaction_type = "";
Review comment:
> Most of these configs are mutable, check them firstly here may be useless. A better way is to check them when any of them are mutated?
OK.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] yangzhg merged pull request #5651: [Compaction] Support to switching cumulative compaction policy dynamically
Posted by GitBox <gi...@apache.org>.
yangzhg merged pull request #5651:
URL: https://github.com/apache/incubator-doris/pull/5651
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org