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 2020/02/17 04:39:02 UTC
[GitHub] [incubator-doris] lingbin opened a new pull request #2918: Some
refactor on `TabletManager`
lingbin opened a new pull request #2918: Some refactor on `TabletManager`
URL: https://github.com/apache/incubator-doris/pull/2918
1. Add some comments to make the code easier to understand;
2. Make the metric `create_tablet_requests_failed` to be accurate;
3. Some internal methods use naked pointers directly instead of `shared_ptr`;
4. Using in `.h` files is contagious when included by other file,
so we should only use it in `.cpp` files;
5. Some formatting changes: such as wrapping lines that are too long
6. Parameters that need to be modified, use pointers instead of references
No functional changes in this patch.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2918: Some refactor on `TabletManager`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2918: Some refactor on `TabletManager`
URL: https://github.com/apache/incubator-doris/pull/2918#discussion_r380005093
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -212,120 +191,133 @@ bool TabletManager::_check_tablet_id_exist_unlocked(TTabletId tablet_id) {
return it != _tablet_map.end() && !it->second.table_arr.empty();
}
-void TabletManager::clear() {
- _tablet_map.clear();
- _shutdown_tablets.clear();
-}
-
OLAPStatus TabletManager::create_tablet(const TCreateTabletReq& request,
std::vector<DataDir*> stores) {
- LOG(INFO) << "begin to process create tablet. tablet=" << request.tablet_id
- << ", schema_hash=" << request.tablet_schema.schema_hash;
- WriteLock wrlock(&_tablet_map_lock);
- OLAPStatus res = OLAP_SUCCESS;
DorisMetrics::create_tablet_requests_total.increment(1);
- // Make sure create_tablet operation is idempotent:
- // return true if tablet with same tablet_id and schema_hash exist,
- // false if tablet with same tablet_id but different schema_hash exist
- // during alter, if the tablet(same tabletid and schema hash) already exist
- // then just return true, if tablet id with different schema hash exist, wait report
- // task to delete the tablet
- if (_check_tablet_id_exist_unlocked(request.tablet_id)) {
- TabletSharedPtr tablet = _get_tablet_unlocked(
- request.tablet_id, request.tablet_schema.schema_hash);
+
+ int64_t tablet_id = request.tablet_id;
+ int32_t schema_hash = request.tablet_schema.schema_hash;
+ LOG(INFO) << "begin to create tablet. tablet_id=" << tablet_id
+ << ", schema_hash=" << schema_hash;
+
+ WriteLock wrlock(&_tablet_map_lock);
+ // Make create_tablet operation to be idempotent:
+ // 1. Return true if tablet with same tablet_id and schema_hash exist;
+ // false if tablet with same tablet_id but different schema_hash exist.
+ // 2. When this is an alter task, if the tablet(both tablet_id and schema_hash are
+ // same) already exist, then just return true(an duplicate request). But if
+ // tablet_id exist but with different schema_hash, return an error(report task will
+ // eventually trigger its deletion).
+ if (_check_tablet_id_exist_unlocked(tablet_id)) {
+ TabletSharedPtr tablet = _get_tablet_unlocked(tablet_id, schema_hash);
if (tablet != nullptr) {
- LOG(INFO) << "create tablet success because tablet already exist. tablet_id="
- << request.tablet_id;
+ LOG(INFO) << "success to create tablet. tablet already exist. tablet_id=" << tablet_id;
return OLAP_SUCCESS;
} else {
- LOG(WARNING) << "tablet with different schema hash already exists. tablet_id="
- << request.tablet_id;
+ LOG(WARNING) << "fail to create tablet. tablet exist but with different schema_hash. "
+ << "tablet_id=" << tablet_id << ", schema_hash=" << schema_hash;
+ DorisMetrics::create_tablet_requests_failed.increment(1);
return OLAP_ERR_CE_TABLET_ID_EXIST;
}
}
- TabletSharedPtr ref_tablet = nullptr;
- bool is_schema_change_tablet = false;
- // if the CreateTabletReq has base_tablet_id then it is a alter tablet request
+ TabletSharedPtr base_tablet = nullptr;
+ bool is_schema_change = false;
+ // If the CreateTabletReq has base_tablet_id then it is a alter-tablet request
if (request.__isset.base_tablet_id && request.base_tablet_id > 0) {
- is_schema_change_tablet = true;
- ref_tablet = _get_tablet_unlocked(request.base_tablet_id, request.base_schema_hash);
- if (ref_tablet == nullptr) {
- LOG(WARNING) << "fail to create new tablet. new_tablet_id=" << request.tablet_id
- << ", new_schema_hash=" << request.tablet_schema.schema_hash
- << ", because could not find base tablet, base_tablet_id=" << request.base_tablet_id
+ is_schema_change = true;
+ base_tablet = _get_tablet_unlocked(request.base_tablet_id, request.base_schema_hash);
+ if (base_tablet == nullptr) {
+ LOG(WARNING) << "fail to create tablet(change schema), base tablet does not exist. "
+ << "new_tablet_id=" << tablet_id << ", new_schema_hash=" << schema_hash
+ << ", base_tablet_id=" << request.base_tablet_id
<< ", base_schema_hash=" << request.base_schema_hash;
+ DorisMetrics::create_tablet_requests_failed.increment(1);
return OLAP_ERR_TABLE_CREATE_META_ERROR;
}
- // schema change should use the same data dir
+ // If we are doing schema-change, we should use the same data dir
+ // TODO(lingbin): A litter trick here, the directory should be determined before
+ // entering this method
stores.clear();
- stores.push_back(ref_tablet->data_dir());
+ stores.push_back(base_tablet->data_dir());
}
- // set alter type to schema change. it is useless
- TabletSharedPtr tablet = _internal_create_tablet_unlocked(AlterTabletType::SCHEMA_CHANGE, request,
- is_schema_change_tablet, ref_tablet, stores);
+ // set alter type to schema-change. it is useless
+ TabletSharedPtr tablet = _internal_create_tablet_unlocked(
+ AlterTabletType::SCHEMA_CHANGE, request, is_schema_change, base_tablet.get(), stores);
if (tablet == nullptr) {
- res = OLAP_ERR_CE_CMD_PARAMS_ERROR;
- LOG(WARNING) << "fail to create tablet. res=" << res;
+ LOG(WARNING) << "fail to create tablet. tablet_id=" << request.tablet_id;
+ DorisMetrics::create_tablet_requests_failed.increment(1);
+ return OLAP_ERR_CE_CMD_PARAMS_ERROR;
}
- LOG(INFO) << "finish to process create tablet. res=" << res;
- return res;
-} // create_tablet
+ LOG(INFO) << "success to create tablet. tablet_id=" << tablet_id
+ << ", schema_hash=" << schema_hash;
+ return OLAP_SUCCESS;
+}
TabletSharedPtr TabletManager::_internal_create_tablet_unlocked(
const AlterTabletType alter_type, const TCreateTabletReq& request,
- const bool is_schema_change_tablet, const TabletSharedPtr ref_tablet,
- std::vector<DataDir*> data_dirs) {
- DCHECK((is_schema_change_tablet && ref_tablet != nullptr)
- || (!is_schema_change_tablet && ref_tablet == nullptr));
- // check if the tablet with specified tablet id and schema hash already exists
- auto checked_tablet = _get_tablet_unlocked(request.tablet_id, request.tablet_schema.schema_hash);
- if (checked_tablet != nullptr) {
- LOG(WARNING) << "failed to create tablet because tablet already exist."
- << " tablet id = " << request.tablet_id
- << " schema hash = " << request.tablet_schema.schema_hash;
- return nullptr;
- }
- bool is_tablet_added = false;
- auto tablet = _create_tablet_meta_and_dir_unlocked(request, is_schema_change_tablet, ref_tablet, data_dirs);
+ const bool is_schema_change, const Tablet* base_tablet,
+ const std::vector<DataDir*>& data_dirs) {
+ // If in schema-change state, base_tablet must also be provided.
+ // i.e., is_schema_change and base_tablet are either assigned or not assigned
+ DCHECK((is_schema_change && base_tablet) || (!is_schema_change && !base_tablet));
+
+ // NOTE: The existence of tablet_id and schema_hash has already been checked,
+ // no need check again here.
+
+ auto tablet = _create_tablet_meta_and_dir_unlocked(request, is_schema_change,
+ base_tablet, data_dirs);
if (tablet == nullptr) {
return nullptr;
}
+ int64_t new_tablet_id = request.tablet_id;
+ int32_t new_schema_hash = request.tablet_schema.schema_hash;
+
+ // should remove the tablet's pending_id no matter create-tablet success or not
+ DataDir* data_dir = tablet->data_dir();
+ std::shared_ptr<void> __defer(nullptr, [&](...) {
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] chaoyli commented on a change in pull request
#2918: Some refactor on `TabletManager`
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #2918: Some refactor on `TabletManager`
URL: https://github.com/apache/incubator-doris/pull/2918#discussion_r379996880
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -83,42 +72,42 @@ OLAPStatus TabletManager::_add_tablet_unlocked(TTabletId tablet_id, SchemaHash s
VLOG(3) << "begin to add tablet to TabletManager. " << "tablet_id=" << tablet_id
<< ", schema_hash=" << schema_hash << ", force=" << force;
- TabletSharedPtr tablet_item = nullptr;
+ TabletSharedPtr exist_tablet = nullptr;
Review comment:
existed_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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin merged pull request #2918: Some refactor
on `TabletManager`
Posted by GitBox <gi...@apache.org>.
lingbin merged pull request #2918: Some refactor on `TabletManager`
URL: https://github.com/apache/incubator-doris/pull/2918
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] lingbin commented on a change in pull request
#2918: Some refactor on `TabletManager`
Posted by GitBox <gi...@apache.org>.
lingbin commented on a change in pull request #2918: Some refactor on `TabletManager`
URL: https://github.com/apache/incubator-doris/pull/2918#discussion_r380005113
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -83,42 +72,42 @@ OLAPStatus TabletManager::_add_tablet_unlocked(TTabletId tablet_id, SchemaHash s
VLOG(3) << "begin to add tablet to TabletManager. " << "tablet_id=" << tablet_id
<< ", schema_hash=" << schema_hash << ", force=" << force;
- TabletSharedPtr tablet_item = nullptr;
+ TabletSharedPtr exist_tablet = nullptr;
Review comment:
Done.
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org
[GitHub] [incubator-doris] chaoyli commented on a change in pull request
#2918: Some refactor on `TabletManager`
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #2918: Some refactor on `TabletManager`
URL: https://github.com/apache/incubator-doris/pull/2918#discussion_r380002318
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -212,120 +191,133 @@ bool TabletManager::_check_tablet_id_exist_unlocked(TTabletId tablet_id) {
return it != _tablet_map.end() && !it->second.table_arr.empty();
}
-void TabletManager::clear() {
- _tablet_map.clear();
- _shutdown_tablets.clear();
-}
-
OLAPStatus TabletManager::create_tablet(const TCreateTabletReq& request,
std::vector<DataDir*> stores) {
- LOG(INFO) << "begin to process create tablet. tablet=" << request.tablet_id
- << ", schema_hash=" << request.tablet_schema.schema_hash;
- WriteLock wrlock(&_tablet_map_lock);
- OLAPStatus res = OLAP_SUCCESS;
DorisMetrics::create_tablet_requests_total.increment(1);
- // Make sure create_tablet operation is idempotent:
- // return true if tablet with same tablet_id and schema_hash exist,
- // false if tablet with same tablet_id but different schema_hash exist
- // during alter, if the tablet(same tabletid and schema hash) already exist
- // then just return true, if tablet id with different schema hash exist, wait report
- // task to delete the tablet
- if (_check_tablet_id_exist_unlocked(request.tablet_id)) {
- TabletSharedPtr tablet = _get_tablet_unlocked(
- request.tablet_id, request.tablet_schema.schema_hash);
+
+ int64_t tablet_id = request.tablet_id;
+ int32_t schema_hash = request.tablet_schema.schema_hash;
+ LOG(INFO) << "begin to create tablet. tablet_id=" << tablet_id
+ << ", schema_hash=" << schema_hash;
+
+ WriteLock wrlock(&_tablet_map_lock);
+ // Make create_tablet operation to be idempotent:
+ // 1. Return true if tablet with same tablet_id and schema_hash exist;
+ // false if tablet with same tablet_id but different schema_hash exist.
+ // 2. When this is an alter task, if the tablet(both tablet_id and schema_hash are
+ // same) already exist, then just return true(an duplicate request). But if
+ // tablet_id exist but with different schema_hash, return an error(report task will
+ // eventually trigger its deletion).
+ if (_check_tablet_id_exist_unlocked(tablet_id)) {
+ TabletSharedPtr tablet = _get_tablet_unlocked(tablet_id, schema_hash);
if (tablet != nullptr) {
- LOG(INFO) << "create tablet success because tablet already exist. tablet_id="
- << request.tablet_id;
+ LOG(INFO) << "success to create tablet. tablet already exist. tablet_id=" << tablet_id;
return OLAP_SUCCESS;
} else {
- LOG(WARNING) << "tablet with different schema hash already exists. tablet_id="
- << request.tablet_id;
+ LOG(WARNING) << "fail to create tablet. tablet exist but with different schema_hash. "
+ << "tablet_id=" << tablet_id << ", schema_hash=" << schema_hash;
+ DorisMetrics::create_tablet_requests_failed.increment(1);
return OLAP_ERR_CE_TABLET_ID_EXIST;
}
}
- TabletSharedPtr ref_tablet = nullptr;
- bool is_schema_change_tablet = false;
- // if the CreateTabletReq has base_tablet_id then it is a alter tablet request
+ TabletSharedPtr base_tablet = nullptr;
+ bool is_schema_change = false;
+ // If the CreateTabletReq has base_tablet_id then it is a alter-tablet request
if (request.__isset.base_tablet_id && request.base_tablet_id > 0) {
- is_schema_change_tablet = true;
- ref_tablet = _get_tablet_unlocked(request.base_tablet_id, request.base_schema_hash);
- if (ref_tablet == nullptr) {
- LOG(WARNING) << "fail to create new tablet. new_tablet_id=" << request.tablet_id
- << ", new_schema_hash=" << request.tablet_schema.schema_hash
- << ", because could not find base tablet, base_tablet_id=" << request.base_tablet_id
+ is_schema_change = true;
+ base_tablet = _get_tablet_unlocked(request.base_tablet_id, request.base_schema_hash);
+ if (base_tablet == nullptr) {
+ LOG(WARNING) << "fail to create tablet(change schema), base tablet does not exist. "
+ << "new_tablet_id=" << tablet_id << ", new_schema_hash=" << schema_hash
+ << ", base_tablet_id=" << request.base_tablet_id
<< ", base_schema_hash=" << request.base_schema_hash;
+ DorisMetrics::create_tablet_requests_failed.increment(1);
return OLAP_ERR_TABLE_CREATE_META_ERROR;
}
- // schema change should use the same data dir
+ // If we are doing schema-change, we should use the same data dir
+ // TODO(lingbin): A litter trick here, the directory should be determined before
+ // entering this method
stores.clear();
- stores.push_back(ref_tablet->data_dir());
+ stores.push_back(base_tablet->data_dir());
}
- // set alter type to schema change. it is useless
- TabletSharedPtr tablet = _internal_create_tablet_unlocked(AlterTabletType::SCHEMA_CHANGE, request,
- is_schema_change_tablet, ref_tablet, stores);
+ // set alter type to schema-change. it is useless
+ TabletSharedPtr tablet = _internal_create_tablet_unlocked(
+ AlterTabletType::SCHEMA_CHANGE, request, is_schema_change, base_tablet.get(), stores);
if (tablet == nullptr) {
- res = OLAP_ERR_CE_CMD_PARAMS_ERROR;
- LOG(WARNING) << "fail to create tablet. res=" << res;
+ LOG(WARNING) << "fail to create tablet. tablet_id=" << request.tablet_id;
+ DorisMetrics::create_tablet_requests_failed.increment(1);
+ return OLAP_ERR_CE_CMD_PARAMS_ERROR;
}
- LOG(INFO) << "finish to process create tablet. res=" << res;
- return res;
-} // create_tablet
+ LOG(INFO) << "success to create tablet. tablet_id=" << tablet_id
+ << ", schema_hash=" << schema_hash;
+ return OLAP_SUCCESS;
+}
TabletSharedPtr TabletManager::_internal_create_tablet_unlocked(
const AlterTabletType alter_type, const TCreateTabletReq& request,
- const bool is_schema_change_tablet, const TabletSharedPtr ref_tablet,
- std::vector<DataDir*> data_dirs) {
- DCHECK((is_schema_change_tablet && ref_tablet != nullptr)
- || (!is_schema_change_tablet && ref_tablet == nullptr));
- // check if the tablet with specified tablet id and schema hash already exists
- auto checked_tablet = _get_tablet_unlocked(request.tablet_id, request.tablet_schema.schema_hash);
- if (checked_tablet != nullptr) {
- LOG(WARNING) << "failed to create tablet because tablet already exist."
- << " tablet id = " << request.tablet_id
- << " schema hash = " << request.tablet_schema.schema_hash;
- return nullptr;
- }
- bool is_tablet_added = false;
- auto tablet = _create_tablet_meta_and_dir_unlocked(request, is_schema_change_tablet, ref_tablet, data_dirs);
+ const bool is_schema_change, const Tablet* base_tablet,
+ const std::vector<DataDir*>& data_dirs) {
+ // If in schema-change state, base_tablet must also be provided.
+ // i.e., is_schema_change and base_tablet are either assigned or not assigned
+ DCHECK((is_schema_change && base_tablet) || (!is_schema_change && !base_tablet));
+
+ // NOTE: The existence of tablet_id and schema_hash has already been checked,
+ // no need check again here.
+
+ auto tablet = _create_tablet_meta_and_dir_unlocked(request, is_schema_change,
+ base_tablet, data_dirs);
if (tablet == nullptr) {
return nullptr;
}
+ int64_t new_tablet_id = request.tablet_id;
+ int32_t new_schema_hash = request.tablet_schema.schema_hash;
+
+ // should remove the tablet's pending_id no matter create-tablet success or not
+ DataDir* data_dir = tablet->data_dir();
+ std::shared_ptr<void> __defer(nullptr, [&](...) {
Review comment:
use SCOPED_CLEANUP instead
----------------------------------------------------------------
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
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org