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 2023/01/19 09:47:00 UTC
[GitHub] [doris] dutyu opened a new pull request, #16100: [feature-wip](profile) add ProfilePlugin and ProfileEventProcessor, w…
dutyu opened a new pull request, #16100:
URL: https://github.com/apache/doris/pull/16100
…hen ProfileManager receive a profile, send this profile to ProfileEventProcessor and handle it if we have installed some ProfilePlugin(s). I will add a ProfileLoader later to save these profile to doris (just like audit log).
# Proposed changes
Issue Number: close #xxx
## Problem summary
Describe your changes.
## Checklist(Required)
1. Does it affect the original behavior:
- [ ] Yes
- [x] No
- [ ] I don't know
2. Has unit tests been added:
- [x] Yes
- [ ] No
- [ ] No Need
3. Has document been added or modified:
- [ ] Yes
- [x] No
- [ ] No Need
4. Does it need to update dependencies:
- [ ] Yes
- [x] No
5. Are there any changes that cannot be rolled back:
- [ ] Yes (If Yes, please explain WHY)
- [x] No
## Further comments
If this is a relatively large or complex change, kick off the discussion at [dev@doris.apache.org](mailto:dev@doris.apache.org) by explaining why you chose the solution you did and what alternatives you considered, 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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
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] [doris] github-actions[bot] commented on a diff in pull request #16100: [feature-wip](profile) add ProfilePlugin and ProfileEventProcessor, w…
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on code in PR #16100:
URL: https://github.com/apache/doris/pull/16100#discussion_r1096932828
##########
be/src/exec/schema_scanner.cpp:
##########
@@ -145,23 +132,161 @@
}
}
-Status SchemaScanner::create_columns(const std::vector<TSchemaTableStructure>* table_structure,
- ObjectPool* pool) {
- _column_num = table_structure->size();
- _columns = new ColumnDesc[_column_num];
- _is_create_columns = true;
- for (size_t idx = 0; idx < table_structure->size(); ++idx) {
- _columns[idx].name = table_structure->at(idx).column_name.c_str();
- _columns[idx].type = thrift_to_type(table_structure->at(idx).type);
- _columns[idx].size = table_structure->at(idx).len;
- _columns[idx].is_null = table_structure->at(idx).is_null;
+Status SchemaScanner::fill_dest_column(vectorized::Block* block, void* data,
+ const ColumnDesc& col_desc) {
+ if (!block->has(col_desc.name)) {
+ return Status::OK();
+ }
+ vectorized::MutableColumnPtr column_ptr =
+ std::move(*block->get_by_name(col_desc.name).column).assume_mutable();
Review Comment:
warning: std::move of the const expression has no effect; remove std::move() [performance-move-const-arg]
```suggestion
*block->get_by_name(col_desc.name).column.assume_mutable();
```
##########
be/src/exec/schema_scanner.cpp:
##########
@@ -33,34 +33,24 @@
#include "exec/schema_scanner/schema_variables_scanner.h"
#include "exec/schema_scanner/schema_views_scanner.h"
#include "runtime/define_primitive_type.h"
+#include "vec/columns/column.h"
#include "vec/common/string_ref.h"
+#include "vec/core/block.h"
namespace doris {
DorisServer* SchemaScanner::_s_doris_server;
-SchemaScanner::SchemaScanner(ColumnDesc* columns, int column_num)
+SchemaScanner::SchemaScanner(const std::vector<ColumnDesc>& columns)
: _is_init(false),
_param(nullptr),
_columns(columns),
- _column_num(column_num),
- _tuple_desc(nullptr),
_schema_table_type(TSchemaTableType::SCH_INVALID) {}
-SchemaScanner::SchemaScanner(ColumnDesc* columns, int column_num, TSchemaTableType::type type)
- : _is_init(false),
- _param(nullptr),
- _columns(columns),
- _column_num(column_num),
- _tuple_desc(nullptr),
- _schema_table_type(type) {}
+SchemaScanner::SchemaScanner(const std::vector<ColumnDesc>& columns, TSchemaTableType::type type)
+ : _is_init(false), _param(nullptr), _columns(columns), _schema_table_type(type) {}
-SchemaScanner::~SchemaScanner() {
- if (_is_create_columns == true && _columns != nullptr) {
- delete[] _columns;
- _columns = nullptr;
- }
-}
+SchemaScanner::~SchemaScanner() {}
Review Comment:
warning: use '= default' to define a trivial destructor [modernize-use-equals-default]
```suggestion
SchemaScanner::~SchemaScanner() = default;
```
##########
be/src/gutil/cpu.cc:
##########
@@ -253,6 +256,7 @@ void CPU::Initialize() {
#endif
}
CPU::IntelMicroArchitecture CPU::GetIntelMicroArchitecture() const {
+ if (has_avx512()) return AVX512;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (has_avx512()) { return AVX512;
}
```
##########
be/src/olap/tablet_meta.cpp:
##########
@@ -877,13 +868,7 @@ bool operator==(const TabletMeta& a, const TabletMeta& b) {
}
if (a._in_restore_mode != b._in_restore_mode) return false;
if (a._preferred_rowset_type != b._preferred_rowset_type) return false;
- if (a._storage_policy != b._storage_policy) return false;
- if (a._cooldown_replica_id != b._cooldown_replica_id) {
- return false;
- }
- if (a._cooldown_term != b._cooldown_term) {
- return false;
- }
+ if (a._storage_policy_id != b._storage_policy_id) return false;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (a._storage_policy_id != b._storage_policy_id) { return false;
}
```
##########
be/src/vec/exec/vschema_scan_node.cpp:
##########
@@ -33,26 +40,12 @@ VSchemaScanNode::VSchemaScanNode(ObjectPool* pool, const TPlanNode& tnode,
_is_init(false),
_table_name(tnode.schema_scan_node.table_name),
_tuple_id(tnode.schema_scan_node.tuple_id),
- _src_tuple_desc(nullptr),
_dest_tuple_desc(nullptr),
_tuple_idx(0),
_slot_num(0),
- _tuple_pool(nullptr),
- _schema_scanner(nullptr),
- _src_tuple(nullptr),
- _src_single_tuple(nullptr),
- _dest_single_tuple(nullptr) {}
-
-VSchemaScanNode::~VSchemaScanNode() {
- delete[] reinterpret_cast<char*>(_src_tuple);
- _src_tuple = nullptr;
+ _schema_scanner(nullptr) {}
- delete[] reinterpret_cast<char*>(_src_single_tuple);
- _src_single_tuple = nullptr;
-
- delete[] reinterpret_cast<char*>(_dest_single_tuple);
- _dest_single_tuple = nullptr;
-}
+VSchemaScanNode::~VSchemaScanNode() {}
Review Comment:
warning: use '= default' to define a trivial destructor [modernize-use-equals-default]
```suggestion
VSchemaScanNode::~VSchemaScanNode() = default;
```
##########
be/src/runtime/result_writer.h:
##########
@@ -33,9 +33,9 @@
// abstract class of the result writer
class ResultWriter {
public:
- ResultWriter() {};
- ResultWriter(bool output_object_data) : _output_object_data(output_object_data) {};
- ~ResultWriter() {};
+ ResultWriter() {}
+ ResultWriter(bool output_object_data) : _output_object_data(output_object_data) {}
+ ~ResultWriter() {}
Review Comment:
warning: use '= default' to define a trivial destructor [modernize-use-equals-default]
```suggestion
~ResultWriter() = default;
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -71,7 +70,6 @@ class StreamLoadActionTest : public testing::Test {
k_response_str = "";
config::streaming_load_max_mb = 1;
- _env._thread_mgr = new ThreadResourceMgr();
_env._master_info = new TMasterInfo();
Review Comment:
warning: '_master_info' is a private member of 'doris::ExecEnv' [clang-diagnostic-error]
```cpp
_env._master_info = new TMasterInfo();
^
```
**be/src/runtime/exec_env.h:225:** declared private here
```cpp
TMasterInfo* _master_info = nullptr;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -71,7 +70,6 @@
k_response_str = "";
config::streaming_load_max_mb = 1;
- _env._thread_mgr = new ThreadResourceMgr();
_env._master_info = new TMasterInfo();
_env._load_stream_mgr = new LoadStreamMgr();
Review Comment:
warning: '_load_stream_mgr' is a private member of 'doris::ExecEnv' [clang-diagnostic-error]
```cpp
_env._load_stream_mgr = new LoadStreamMgr();
^
```
**be/src/runtime/exec_env.h:232:** declared private here
```cpp
LoadStreamMgr* _load_stream_mgr = nullptr;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -89,8 +87,6 @@
_env._load_stream_mgr = nullptr;
delete _env._master_info;
_env._master_info = nullptr;
Review Comment:
warning: '_master_info' is a private member of 'doris::ExecEnv' [clang-diagnostic-error]
```cpp
_env._master_info = nullptr;
^
```
**be/src/runtime/exec_env.h:225:** declared private here
```cpp
TMasterInfo* _master_info = nullptr;
^
```
##########
be/src/io/fs/s3_file_system.h:
##########
@@ -68,19 +68,17 @@ class S3FileSystem final : public RemoteFileSystem {
std::shared_ptr<Aws::S3::S3Client> get_client() const {
std::lock_guard lock(_client_mu);
return _client;
- };
+ }
// Guarded by external lock.
- void set_ak(std::string ak) { _s3_conf.ak = std::move(ak); }
-
- // Guarded by external lock.
- void set_sk(std::string sk) { _s3_conf.sk = std::move(sk); }
+ void set_conf(S3Conf s3_conf) { _s3_conf = std::move(s3_conf); }
std::string get_key(const Path& path) const;
private:
- S3FileSystem(S3Conf s3_conf, ResourceId resource_id);
+ S3FileSystem(S3Conf&& s3_conf, std::string&& id);
+private:
Review Comment:
warning: redundant access specifier has the same accessibility as the previous access specifier [readability-redundant-access-specifiers]
```suggestion
```
**be/src/io/fs/s3_file_system.h:77:** previously declared here
```cpp
private:
^
```
##########
be/src/olap/cumulative_compaction_policy.cpp:
##########
@@ -334,13 +324,12 @@ int SizeBasedCumulativeCompactionPolicy::pick_input_rowsets(
return transient_size;
}
-int SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
- for (auto& i : _levels) {
- if (size >= i) {
- return i;
- }
- }
- return 0;
+int64_t SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
+ if (size < 1024) return 0;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (size < 1024) { return 0;
}
```
##########
be/src/olap/cumulative_compaction_policy.cpp:
##########
@@ -334,13 +324,12 @@
return transient_size;
}
-int SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
- for (auto& i : _levels) {
- if (size >= i) {
- return i;
- }
- }
- return 0;
+int64_t SizeBasedCumulativeCompactionPolicy::_level_size(const int64_t size) {
+ if (size < 1024) return 0;
+ int64_t max_level = (int64_t)1
+ << (sizeof(_promotion_size) * 8 - 1 - __builtin_clzl(_promotion_size / 2));
+ if (size >= max_level) return max_level;
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (size >= max_level) { return max_level;
}
```
##########
be/src/runtime/result_writer.h:
##########
@@ -33,9 +33,9 @@ class Block;
// abstract class of the result writer
class ResultWriter {
public:
- ResultWriter() {};
- ResultWriter(bool output_object_data) : _output_object_data(output_object_data) {};
- ~ResultWriter() {};
+ ResultWriter() {}
Review Comment:
warning: use '= default' to define a trivial default constructor [modernize-use-equals-default]
```suggestion
ResultWriter() = default;
```
##########
be/src/runtime/memory/mem_tracker_limiter.cpp:
##########
@@ -195,26 +196,35 @@ void MemTrackerLimiter::print_log_usage(const std::string& msg) {
}
}
+std::string MemTrackerLimiter::log_process_usage_str(const std::string& msg, bool with_stacktrace) {
+ std::string detail = msg;
+ detail += "\nProcess Memory Summary:\n " + MemTrackerLimiter::process_mem_log_str();
+ if (with_stacktrace) detail += "\nAlloc Stacktrace:\n" + get_stack_trace();
Review Comment:
warning: statement should be inside braces [readability-braces-around-statements]
```suggestion
if (with_stacktrace) { detail += "\nAlloc Stacktrace:\n" + get_stack_trace();
}
```
##########
be/test/agent/cgroups_mgr_test.cpp:
##########
@@ -21,13 +21,8 @@
#include <filesystem>
#include <fstream>
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#ifndef BE_TEST
-#define BE_TEST
-#endif
-
using ::testing::_;
using ::testing::Return;
Review Comment:
warning: no member named 'Return' in namespace 'testing' [clang-diagnostic-error]
```cpp
using ::testing::Return;
^
```
##########
be/src/vec/exec/vschema_scan_node.cpp:
##########
@@ -255,254 +212,70 @@
return Status::InternalError("used before initialize.");
}
RETURN_IF_CANCELLED(state);
- std::vector<vectorized::MutableColumnPtr> columns(_slot_num);
bool schema_eos = false;
- do {
- bool mem_reuse = block->mem_reuse();
- DCHECK(block->rows() == 0);
+ const std::vector<SchemaScanner::ColumnDesc>& columns_desc(_schema_scanner->get_column_desc());
- columns.resize(_slot_num);
+ do {
+ block->clear();
for (int i = 0; i < _slot_num; ++i) {
- if (mem_reuse) {
- columns[i] = std::move(*block->get_by_position(i).column).mutate();
- } else {
- columns[i] = _dest_tuple_desc->slots()[i]->get_empty_mutable_column();
- }
+ auto dest_slot_desc = _dest_tuple_desc->slots()[i];
+ block->insert(ColumnWithTypeAndName(dest_slot_desc->get_empty_mutable_column(),
+ dest_slot_desc->get_data_type_ptr(),
+ dest_slot_desc->col_name()));
+ }
+
+ vectorized::Block src_block;
+ for (int i = 0; i < columns_desc.size(); ++i) {
+ TypeDescriptor descriptor(columns_desc[i].type);
+ auto data_type =
+ vectorized::DataTypeFactory::instance().create_data_type(descriptor, true);
+ src_block.insert(ColumnWithTypeAndName(data_type->create_column(), data_type,
+ columns_desc[i].name));
}
while (true) {
RETURN_IF_CANCELLED(state);
// get all slots from schema table.
- RETURN_IF_ERROR(_schema_scanner->get_next_row(_src_single_tuple, _tuple_pool.get(),
- &schema_eos));
+ RETURN_IF_ERROR(_schema_scanner->get_next_block(&src_block, &schema_eos));
+
if (schema_eos) {
*eos = true;
break;
}
- // tuple project
- project_tuple();
- for (int i = 0; i < _slot_num; ++i) {
- auto slot_desc = _dest_tuple_desc->slots()[i];
- if (!slot_desc->is_materialized()) {
- continue;
- }
-
- if (_dest_single_tuple->is_null(slot_desc->null_indicator_offset())) {
- if (slot_desc->is_nullable()) {
- auto* nullable_column =
- reinterpret_cast<vectorized::ColumnNullable*>(columns[i].get());
- nullable_column->insert_data(nullptr, 0);
- } else {
- return Status::InternalError(
- "nonnull column contains NULL. table={}, column={}", _table_name,
- slot_desc->col_name());
- }
- } else {
- RETURN_IF_ERROR(write_slot_to_vectorized_column(
- _dest_single_tuple->get_slot(slot_desc->tuple_offset()), slot_desc,
- &columns[i]));
- }
- }
- if (columns[0]->size() == state->batch_size()) {
+ if (src_block.rows() >= state->batch_size()) {
break;
}
}
- if (!columns.empty() && !columns[0]->empty()) {
- auto n_columns = 0;
- if (!mem_reuse) {
- for (const auto slot_desc : _dest_tuple_desc->slots()) {
- block->insert(ColumnWithTypeAndName(std::move(columns[n_columns++]),
- slot_desc->get_data_type_ptr(),
- slot_desc->col_name()));
- }
- } else {
- columns.clear();
+
+ if (src_block.rows()) {
+ // block->check_number_of_rows();
+ for (int i = 0; i < _slot_num; ++i) {
+ auto dest_slot_desc = _dest_tuple_desc->slots()[i];
+ vectorized::MutableColumnPtr column_ptr =
+ std::move(*block->get_by_position(i).column).mutate();
Review Comment:
warning: std::move of the const expression has no effect; remove std::move() [performance-move-const-arg]
```suggestion
*block->get_by_position(i).column.mutate();
```
##########
be/src/util/mem_info.cpp:
##########
@@ -47,6 +47,7 @@ int64_t MemInfo::_s_physical_mem = -1;
int64_t MemInfo::_s_mem_limit = -1;
std::string MemInfo::_s_mem_limit_str = "";
int64_t MemInfo::_s_soft_mem_limit = -1;
+std::string MemInfo::_s_soft_mem_limit_str = "";
Review Comment:
warning: redundant string initialization [readability-redundant-string-init]
```suggestion
std::string MemInfo::_s_soft_mem_limit_str;
```
##########
be/test/agent/cgroups_mgr_test.cpp:
##########
@@ -21,13 +21,8 @@
#include <filesystem>
#include <fstream>
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#ifndef BE_TEST
-#define BE_TEST
-#endif
-
using ::testing::_;
using ::testing::Return;
using ::testing::SetArgPointee;
Review Comment:
warning: no member named 'SetArgPointee' in namespace 'testing' [clang-diagnostic-error]
```cpp
using ::testing::SetArgPointee;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -71,7 +70,6 @@
k_response_str = "";
config::streaming_load_max_mb = 1;
- _env._thread_mgr = new ThreadResourceMgr();
_env._master_info = new TMasterInfo();
_env._load_stream_mgr = new LoadStreamMgr();
_env._internal_client_cache = new BrpcClientCache<PBackendService_Stub>();
Review Comment:
warning: '_internal_client_cache' is a private member of 'doris::ExecEnv' [clang-diagnostic-error]
```cpp
_env._internal_client_cache = new BrpcClientCache<PBackendService_Stub>();
^
```
**be/src/runtime/exec_env.h:234:** declared private here
```cpp
BrpcClientCache<PBackendService_Stub>* _internal_client_cache = nullptr;
^
```
##########
be/test/agent/cgroups_mgr_test.cpp:
##########
@@ -21,13 +21,8 @@
#include <filesystem>
#include <fstream>
-#include "gmock/gmock.h"
#include "gtest/gtest.h"
-#ifndef BE_TEST
-#define BE_TEST
-#endif
-
using ::testing::_;
Review Comment:
warning: no member named '_' in namespace 'testing' [clang-diagnostic-error]
```cpp
using ::testing::_;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -89,8 +87,6 @@
_env._load_stream_mgr = nullptr;
delete _env._master_info;
Review Comment:
warning: '_master_info' is a private member of 'doris::ExecEnv' [clang-diagnostic-error]
```cpp
delete _env._master_info;
^
```
**be/src/runtime/exec_env.h:225:** declared private here
```cpp
TMasterInfo* _master_info = nullptr;
^
```
##########
be/test/http/stream_load_test.cpp:
##########
@@ -89,8 +87,6 @@
_env._load_stream_mgr = nullptr;
Review Comment:
warning: '_load_stream_mgr' is a private member of 'doris::ExecEnv' [clang-diagnostic-error]
```cpp
_env._load_stream_mgr = nullptr;
^
```
**be/src/runtime/exec_env.h:232:** declared private here
```cpp
LoadStreamMgr* _load_stream_mgr = 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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
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] [doris] dutyu closed pull request #16100: [feature-wip](profile) add ProfilePlugin and ProfileEventProcessor, w…
Posted by "dutyu (via GitHub)" <gi...@apache.org>.
dutyu closed pull request #16100: [feature-wip](profile) add ProfilePlugin and ProfileEventProcessor, w…
URL: https://github.com/apache/doris/pull/16100
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
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] [doris] hello-stephen commented on pull request #16100: [feature-wip](profile) add ProfilePlugin and ProfileEventProcessor, w…
Posted by GitBox <gi...@apache.org>.
hello-stephen commented on PR #16100:
URL: https://github.com/apache/doris/pull/16100#issuecomment-1396829734
TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 35.86 seconds
load time: 500 seconds
storage size: 17122131703 Bytes
https://doris-community-test-1308700295.cos.ap-hongkong.myqcloud.com/tmp/20230119112759_clickbench_pr_84121.html
--
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.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
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