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/04/17 03:04:54 UTC
[GitHub] [incubator-doris] acelyc111 opened a new pull request #3339:
[tablet] A small refactor on class Tablet
acelyc111 opened a new pull request #3339: [tablet] A small refactor on class Tablet
URL: https://github.com/apache/incubator-doris/pull/3339
There is no functional changes in this patch.
Key refactor points are:
- Remove meaningless return value of functions in class Tablet, and
also some related functions in other classes
- Allow RowsetGraph::capture_consistent_versions to pass a nullptr
to the output parameter
- Use CHECK instead of LOG(FATAL) to simplify code
----------------------------------------------------------------
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] acelyc111 commented on a change in pull request
#3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r409997530
##########
File path: be/src/olap/rowset_graph.cpp
##########
@@ -237,25 +232,26 @@ OLAPStatus RowsetGraph::capture_consistent_versions(const Version& spec_version,
reversed_path.push_back(tmp_vertex_index);
}
- // Make version_path from reversed_path.
- std::stringstream shortest_path_for_debug;
- for (size_t path_id = reversed_path.size() - 1; path_id > 0; --path_id) {
- int64_t tmp_start_vertex_value = _version_graph[reversed_path[path_id]].value;
- int64_t tmp_end_vertex_value = _version_graph[reversed_path[path_id - 1]].value;
-
- // tmp_start_vertex_value mustn't be equal to tmp_end_vertex_value
- if (tmp_start_vertex_value <= tmp_end_vertex_value) {
- version_path->emplace_back(tmp_start_vertex_value, tmp_end_vertex_value - 1);
- } else {
- version_path->emplace_back(tmp_end_vertex_value, tmp_start_vertex_value - 1);
- }
+ if (version_path != nullptr) {
Review comment:
I found `RowsetGraph::capture_consistent_versions` is now only called by `Tablet::capture_consistent_versions`, the latter is now called by two purposes. First, check version integrity, second, check version integrity and then capture versions when check success. For the first purpose, it's not needed to construct and fill a output parameter. Of course the function name `capture_consistent_versions ` maybe not accuracy in this case.
----------------------------------------------------------------
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
#3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r409985554
##########
File path: be/src/olap/rowset_graph.cpp
##########
@@ -237,25 +232,26 @@ OLAPStatus RowsetGraph::capture_consistent_versions(const Version& spec_version,
reversed_path.push_back(tmp_vertex_index);
}
- // Make version_path from reversed_path.
- std::stringstream shortest_path_for_debug;
- for (size_t path_id = reversed_path.size() - 1; path_id > 0; --path_id) {
- int64_t tmp_start_vertex_value = _version_graph[reversed_path[path_id]].value;
- int64_t tmp_end_vertex_value = _version_graph[reversed_path[path_id - 1]].value;
-
- // tmp_start_vertex_value mustn't be equal to tmp_end_vertex_value
- if (tmp_start_vertex_value <= tmp_end_vertex_value) {
- version_path->emplace_back(tmp_start_vertex_value, tmp_end_vertex_value - 1);
- } else {
- version_path->emplace_back(tmp_end_vertex_value, tmp_start_vertex_value - 1);
- }
+ if (version_path != nullptr) {
Review comment:
I think should not coupling outside VersionPath empty logic in this function.
----------------------------------------------------------------
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
#3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r409981960
##########
File path: be/src/olap/tablet.cpp
##########
@@ -135,16 +130,12 @@ OLAPStatus Tablet::set_tablet_state(TabletState state) {
// should save tablet meta to remote meta store
// if it's a primary replica
-OLAPStatus Tablet::save_meta() {
- OLAPStatus res = _tablet_meta->save_meta(_data_dir);
- if (res != OLAP_SUCCESS) {
- LOG(FATAL) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
- }
+void Tablet::save_meta() {
+ auto res = _tablet_meta->save_meta(_data_dir);
+ CHECK_EQ(res, OLAP_SUCCESS) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
Review comment:
CHECK_EQ takes no effect on release version.
----------------------------------------------------------------
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
#3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r409985592
##########
File path: be/src/olap/rowset_graph.cpp
##########
@@ -159,11 +159,6 @@ OLAPStatus RowsetGraph::capture_consistent_versions(const Version& spec_version,
return OLAP_ERR_INPUT_PARAMETER_ERROR;
}
Review comment:
You can CHECK(VERSION_PATH != nullptr) to replace above logic.
----------------------------------------------------------------
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] acelyc111 commented on a change in pull request
#3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r409993014
##########
File path: be/src/olap/tablet.cpp
##########
@@ -135,16 +130,12 @@ OLAPStatus Tablet::set_tablet_state(TabletState state) {
// should save tablet meta to remote meta store
// if it's a primary replica
-OLAPStatus Tablet::save_meta() {
- OLAPStatus res = _tablet_meta->save_meta(_data_dir);
- if (res != OLAP_SUCCESS) {
- LOG(FATAL) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
- }
+void Tablet::save_meta() {
+ auto res = _tablet_meta->save_meta(_data_dir);
+ CHECK_EQ(res, OLAP_SUCCESS) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
Review comment:
> CHECK_EQ takes no effect on release version.
Do you mean **D**CHECK_EQ takes no effect on release version?
----------------------------------------------------------------
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] acelyc111 commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #3339:
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r413844614
##########
File path: be/src/olap/rowset_graph.cpp
##########
@@ -237,25 +232,26 @@ OLAPStatus RowsetGraph::capture_consistent_versions(const Version& spec_version,
reversed_path.push_back(tmp_vertex_index);
}
- // Make version_path from reversed_path.
- std::stringstream shortest_path_for_debug;
- for (size_t path_id = reversed_path.size() - 1; path_id > 0; --path_id) {
- int64_t tmp_start_vertex_value = _version_graph[reversed_path[path_id]].value;
- int64_t tmp_end_vertex_value = _version_graph[reversed_path[path_id - 1]].value;
-
- // tmp_start_vertex_value mustn't be equal to tmp_end_vertex_value
- if (tmp_start_vertex_value <= tmp_end_vertex_value) {
- version_path->emplace_back(tmp_start_vertex_value, tmp_end_vertex_value - 1);
- } else {
- version_path->emplace_back(tmp_end_vertex_value, tmp_start_vertex_value - 1);
- }
+ if (version_path != nullptr) {
Review comment:
Either `version_path` is nullptr or is valid for this function. If it's nullptr, just validate rowset graph; if it's not nullptr, validate rowset graph and return current versions.
----------------------------------------------------------------
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 #3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on a change in pull request #3339:
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r412670837
##########
File path: be/src/olap/tablet.cpp
##########
@@ -135,16 +130,12 @@ OLAPStatus Tablet::set_tablet_state(TabletState state) {
// should save tablet meta to remote meta store
// if it's a primary replica
-OLAPStatus Tablet::save_meta() {
- OLAPStatus res = _tablet_meta->save_meta(_data_dir);
- if (res != OLAP_SUCCESS) {
- LOG(FATAL) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
- }
+void Tablet::save_meta() {
+ auto res = _tablet_meta->save_meta(_data_dir);
+ CHECK_EQ(res, OLAP_SUCCESS) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
Review comment:
`DCHECK_EQ` has no effect, but here is `CHECK_EQ ` which has effect in release mode.
----------------------------------------------------------------
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 issue #3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
acelyc111 commented on issue #3339:
URL: https://github.com/apache/incubator-doris/pull/3339#issuecomment-617518169
> Compile failed.
Fixed.
----------------------------------------------------------------
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] chaoyli commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3339:
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r412622055
##########
File path: be/src/olap/rowset_graph.cpp
##########
@@ -237,25 +232,26 @@ OLAPStatus RowsetGraph::capture_consistent_versions(const Version& spec_version,
reversed_path.push_back(tmp_vertex_index);
}
- // Make version_path from reversed_path.
- std::stringstream shortest_path_for_debug;
- for (size_t path_id = reversed_path.size() - 1; path_id > 0; --path_id) {
- int64_t tmp_start_vertex_value = _version_graph[reversed_path[path_id]].value;
- int64_t tmp_end_vertex_value = _version_graph[reversed_path[path_id - 1]].value;
-
- // tmp_start_vertex_value mustn't be equal to tmp_end_vertex_value
- if (tmp_start_vertex_value <= tmp_end_vertex_value) {
- version_path->emplace_back(tmp_start_vertex_value, tmp_end_vertex_value - 1);
- } else {
- version_path->emplace_back(tmp_end_vertex_value, tmp_start_vertex_value - 1);
- }
+ if (version_path != nullptr) {
Review comment:
I think there is no necessity for capture_consistent_versions() judge version_path is nullptr or not.
So you can DCHECK(version_path != nullptr) instead.
Of course, you should guarantee that the caller should the version_path is not nullptr.
----------------------------------------------------------------
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] chaoyli commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3339:
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r412618322
##########
File path: be/src/olap/tablet.cpp
##########
@@ -135,16 +130,12 @@ OLAPStatus Tablet::set_tablet_state(TabletState state) {
// should save tablet meta to remote meta store
// if it's a primary replica
-OLAPStatus Tablet::save_meta() {
- OLAPStatus res = _tablet_meta->save_meta(_data_dir);
- if (res != OLAP_SUCCESS) {
- LOG(FATAL) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
- }
+void Tablet::save_meta() {
+ auto res = _tablet_meta->save_meta(_data_dir);
+ CHECK_EQ(res, OLAP_SUCCESS) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
Review comment:
Yes
----------------------------------------------------------------
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] chaoyli commented on a change in pull request #3339: [tablet] A small refactor on class Tablet
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3339:
URL: https://github.com/apache/incubator-doris/pull/3339#discussion_r412618322
##########
File path: be/src/olap/tablet.cpp
##########
@@ -135,16 +130,12 @@ OLAPStatus Tablet::set_tablet_state(TabletState state) {
// should save tablet meta to remote meta store
// if it's a primary replica
-OLAPStatus Tablet::save_meta() {
- OLAPStatus res = _tablet_meta->save_meta(_data_dir);
- if (res != OLAP_SUCCESS) {
- LOG(FATAL) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
- }
+void Tablet::save_meta() {
+ auto res = _tablet_meta->save_meta(_data_dir);
+ CHECK_EQ(res, OLAP_SUCCESS) << "fail to save tablet_meta. res=" << res << ", root=" << _data_dir->path();
Review comment:
Yes, It has no effect.
----------------------------------------------------------------
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