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/06/03 11:49:04 UTC
[GitHub] [incubator-doris] decster opened a new pull request #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
decster opened a new pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762
After adding meta, BE now can create MemTablet, and put it into TabletManager, but MemTablet is not compatible with Tablet, a lot of code may break. This CL contains the following changes:
When TabletManager::get_tablet is called, only return Tablet, if the underlying tablet is MemTablet, return an error, this keeps the initial code changes small.
For methods/functionalities Tablet&MemTablet both have, refactor/extract them to BaseTablet, and change the usage to TabletManager::get_base_tablet, and it will gradually remove errors introduced by step1, this is a long going process.
----------------------------------------------------------------
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 pull request #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
morningman commented on pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#issuecomment-640690789
> > Hi @decster , in your comment, you said `When TabletManager::get_tablet is called, only return Tablet, if the underlying tablet is MemTablet, return an error, this keeps the initial code changes small.`
> > But i didn't see the code change related to it?
>
> the related change is in tablet_manager TabletManager::_get_tablet_unlocked
>
> ```
> if (ret->is_memory()) {
> LOG(FATAL) << "_get_tablet_unlocked get MemTablet";
> return TabletSharedPtr();
> }
> ```
OK, I see~
----------------------------------------------------------------
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 #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
chaoyli commented on a change in pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#discussion_r437093426
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -1374,16 +1482,29 @@ TabletSharedPtr TabletManager::_get_tablet_unlocked(TTabletId tablet_id, SchemaH
VLOG(3) << "fail to get tablet. tablet_id=" << tablet_id << ", schema_hash=" << schema_hash;
// Return nullptr tablet if fail
- TabletSharedPtr tablet;
+ BaseTabletSharedPtr tablet;
Review comment:
You can return nullptr directly.
----------------------------------------------------------------
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 #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#discussion_r436788740
##########
File path: be/src/olap/tablet_manager.cpp
##########
@@ -98,26 +99,34 @@ OLAPStatus TabletManager::_add_tablet_unlocked(TTabletId tablet_id, SchemaHash s
if (existed_tablet == nullptr) {
return _add_tablet_to_map_unlocked(tablet_id, schema_hash,
- tablet, update_meta,
+ base_tablet, update_meta,
false /*keep_files*/, false /*drop_old*/);
}
if (!force) {
- if (existed_tablet->tablet_path() == tablet->tablet_path()) {
+ if (existed_tablet->tablet_path() == base_tablet->tablet_path()) {
LOG(WARNING) << "add the same tablet twice! tablet_id=" << tablet_id
<< ", schema_hash=" << schema_hash
- << ", tablet_path=" << tablet->tablet_path();
+ << ", tablet_path=" << base_tablet->tablet_path();
return OLAP_ERR_ENGINE_INSERT_EXISTS_TABLE;
}
- if (existed_tablet->data_dir() == tablet->data_dir()) {
+ if (existed_tablet->data_dir() == base_tablet->data_dir()) {
LOG(WARNING) << "add tablet with same data dir twice! tablet_id=" << tablet_id
<< ", schema_hash=" << schema_hash;
return OLAP_ERR_ENGINE_INSERT_EXISTS_TABLE;
}
}
+ if (base_tablet->is_memory() || existed_tablet->is_memory()) {
+ LOG(WARNING) << "add the same tablet twice! tablet_id=" << tablet_id
Review comment:
Change the comment
----------------------------------------------------------------
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 pull request #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
morningman commented on pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#issuecomment-639558306
Hi @decster , in your comment, you said `When TabletManager::get_tablet is called, only return Tablet, if the underlying tablet is MemTablet, return an error, this keeps the initial code changes small.`
But i didn't see the code change related to it?
----------------------------------------------------------------
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] decster commented on pull request #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
decster commented on pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#issuecomment-640343996
> Hi @decster , in your comment, you said `When TabletManager::get_tablet is called, only return Tablet, if the underlying tablet is MemTablet, return an error, this keeps the initial code changes small.`
>
> But i didn't see the code change related to it?
the related change is in tablet_manager TabletManager::_get_tablet_unlocked
```
if (ret->is_memory()) {
LOG(FATAL) << "_get_tablet_unlocked get MemTablet";
return TabletSharedPtr();
}
```
----------------------------------------------------------------
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 merged pull request #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
chaoyli merged pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762
----------------------------------------------------------------
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] decster commented on a change in pull request #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
decster commented on a change in pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#discussion_r436441988
##########
File path: be/src/olap/tablet.h
##########
@@ -46,47 +46,27 @@ class TabletMeta;
using TabletSharedPtr = std::shared_ptr<Tablet>;
+inline TabletSharedPtr to_tablet(const BaseTabletSharedPtr& base) {
+ if (base->is_memory()) {
+ return TabletSharedPtr();
Review comment:
Why is this incomplete? This utility function for pointer conversion is only used in some internal code, another option is inline them, because, after inlining, some logic is redundant.
----------------------------------------------------------------
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 #3762: [Memory Engine] MemTablet creation and compatibility handling in BE
Posted by GitBox <gi...@apache.org>.
morningman commented on a change in pull request #3762:
URL: https://github.com/apache/incubator-doris/pull/3762#discussion_r435986050
##########
File path: be/src/olap/memory/mem_tablet.h
##########
@@ -27,6 +27,15 @@ class MemSubTablet;
class ScanSpec;
class MemTabletScan;
class WriteTxn;
+class MemTablet;
+using MemTabletSharedPtr = std::shared_ptr<MemTablet>;
+
+inline MemTabletSharedPtr to_mem_tablet(const BaseTabletSharedPtr& base) {
+ if (base->is_memory()) {
+ return std::static_pointer_cast<MemTablet>(base);
+ }
+ return MemTabletSharedPtr();
Review comment:
Is this a incomplete implementation? If yes, better add a TODO
##########
File path: be/src/olap/tablet.h
##########
@@ -46,47 +46,27 @@ class TabletMeta;
using TabletSharedPtr = std::shared_ptr<Tablet>;
+inline TabletSharedPtr to_tablet(const BaseTabletSharedPtr& base) {
+ if (base->is_memory()) {
+ return TabletSharedPtr();
Review comment:
incomplete?
----------------------------------------------------------------
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