morningman commented on code in PR #8855:
URL: https://github.com/apache/incubator-doris/pull/8855#discussion_r847462849
##########
be/src/olap/delete_handler.cpp:
##########
@@ -48,20 +48,20 @@ using google::protobuf::RepeatedPtrField;
namespace doris {
-OLAPStatus DeleteConditionHandler::generate_delete_predicate(
+Status DeleteConditionHandler::generate_delete_predicate(
const TabletSchema& schema, const std::vector<TCondition>& conditions,
DeletePredicatePB* del_pred) {
if (conditions.empty()) {
LOG(WARNING) << "invalid parameters for store_cond."
<< " condition_size=" << conditions.size();
- return OLAP_ERR_DELETE_INVALID_PARAMETERS;
+ return Status::OLAPInternalError(OLAP_ERR_DELETE_INVALID_PARAMETERS);
}
// Check whether the delete condition meets the requirements
for (const TCondition& condition : conditions) {
- if (check_condition_valid(schema, condition) != OLAP_SUCCESS) {
+ if (check_condition_valid(schema, condition) != Status::OK()) {
LOG(WARNING) << "invalid condition. condition=" <<
ThriftDebugString(condition);
- return OLAP_ERR_DELETE_INVALID_CONDITION;
+ return
Status::OLAPInternalError(OLAP_ERR_DELETE_INVALID_CONDITION);
Review Comment:
should return the result of `check_condition_valid(schema, condition)`?
##########
be/src/olap/memtable_flush_executor.cpp:
##########
@@ -67,7 +71,10 @@ void FlushToken::_flush_memtable(std::shared_ptr<MemTable>
memtable, int64_t sub
MonotonicStopWatch timer;
timer.start();
- _flush_status.store(memtable->flush());
+ Status s = memtable->flush();
+ LOG(WARNING) << "Flush memtable failed with res = " << s;
+ // If s is not ok, ignore the code, just use other code is ok
+ _flush_status.store(s.ok() ? OLAP_SUCCESS : OLAP_ERR_OTHER_ERROR);
Review Comment:
Why not using the code from `s`?
##########
be/src/http/action/compaction_action.cpp:
##########
@@ -207,44 +207,42 @@ OLAPStatus
CompactionAction::_execute_compaction_callback(TabletSharedPtr tablet
tablet->set_cumulative_compaction_policy(cumulative_compaction_policy);
}
- OLAPStatus status = OLAP_SUCCESS;
+ Status res = Status::OK();
if (compaction_type == PARAM_COMPACTION_BASE) {
BaseCompaction base_compaction(tablet);
- OLAPStatus res = base_compaction.compact();
- if (res != OLAP_SUCCESS) {
- if (res == OLAP_ERR_BE_NO_SUITABLE_VERSION) {
+ res = base_compaction.compact();
+ if (!res) {
+ if (res ==
Status::OLAPInternalError(OLAP_ERR_BE_NO_SUITABLE_VERSION)) {
Review Comment:
Here you call `Status::OLAPInternalError` only for checking the error code?
Why not do thing like `if (res.err_code == OLAP_ERR_BE_NO_SUITABLE_VERSION)`
##########
be/src/vec/exec/volap_scanner.cpp:
##########
@@ -57,12 +57,12 @@ Status VOlapScanner::get_block(RuntimeState* state,
vectorized::Block* block, bo
do {
// Read one block from block reader
auto res = _tablet_reader->next_block_with_aggregation(block,
nullptr, nullptr, eof);
- if (res != OLAP_SUCCESS) {
+ if (!res) {
std::stringstream ss;
- ss << "Internal Error: read storage fail. res=" << res
+ ss << "Internal Error: read storage fail. res=" <<
res.to_string()
Review Comment:
this `ss` can be removed
##########
be/src/olap/task/engine_batch_load_task.cpp:
##########
@@ -257,12 +256,12 @@ Status EngineBatchLoadTask::_process() {
if (status.ok()) {
// Load delta file
time_t push_begin = time(nullptr);
- OLAPStatus push_status = _push(_push_req, _tablet_infos);
+ Status push_status = _push(_push_req, _tablet_infos);
time_t push_finish = time(nullptr);
LOG(INFO) << "Push finish, cost time: " << (push_finish - push_begin);
- if (push_status ==
OLAPStatus::OLAP_ERR_PUSH_TRANSACTION_ALREADY_EXIST) {
+ if (push_status ==
Status::OLAPInternalError(OLAP_ERR_PUSH_TRANSACTION_ALREADY_EXIST)) {
status = Status::OK();
- } else if (push_status != OLAPStatus::OLAP_SUCCESS) {
+ } else if (push_status != Status::OK()) {
status = Status::InternalError("Unknown");
Review Comment:
```suggestion
status = push_status;
```
##########
be/src/olap/memtable_flush_executor.cpp:
##########
@@ -67,7 +71,10 @@ void FlushToken::_flush_memtable(std::shared_ptr<MemTable>
memtable, int64_t sub
MonotonicStopWatch timer;
timer.start();
- _flush_status.store(memtable->flush());
+ Status s = memtable->flush();
+ LOG(WARNING) << "Flush memtable failed with res = " << s;
Review Comment:
```
if (!s) {
LOG(WARNING) << .....
}
```
##########
be/src/olap/task/engine_batch_load_task.cpp:
##########
@@ -80,17 +80,16 @@ OLAPStatus EngineBatchLoadTask::execute() {
}
}
} else if (_push_req.push_type == TPushType::DELETE) {
- OLAPStatus delete_data_status = _delete_data(_push_req, _tablet_infos);
- if (delete_data_status != OLAPStatus::OLAP_SUCCESS) {
- OLAP_LOG_WARNING("delete data failed. status: %d, signature: %ld",
delete_data_status,
- _signature);
+ Status delete_data_status = _delete_data(_push_req, _tablet_infos);
+ if (delete_data_status != Status::OK()) {
+ LOG(WARNING) << "delete data failed. status:" <<
delete_data_status << " signature:" << _signature;
status = Status::InternalError("Delete data failed");
Review Comment:
```suggestion
status = delete_data_status;
```
##########
be/src/olap/rowset/CMakeLists.txt:
##########
@@ -46,3 +46,5 @@ add_library(Rowset STATIC
beta_rowset.cpp
beta_rowset_reader.cpp
beta_rowset_writer.cpp)
+
+target_compile_options(Rowset PUBLIC "-Wno-error=maybe-uninitialized")
Review Comment:
Why adding this?
--
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-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
