xiaokang commented on code in PR #33076:
URL: https://github.com/apache/doris/pull/33076#discussion_r1548867186
##########
be/src/http/action/compaction_action.cpp:
##########
@@ -278,6 +279,23 @@ Status
CompactionAction::_execute_compaction_callback(TabletSharedPtr tablet,
<< ", table=" << tablet->tablet_id();
}
}
+ } else if (compaction_type == PARAM_COMPACTION_REMOTE) {
+ do {
+ if
(!tablet->tablet_meta()->tablet_schema()->enable_single_replica_compaction()) {
+ res = Status::Cancelled("tablet is not enable single replica
compaction");
+ break;
+ }
+ if (!_engine.should_fetch_from_peer(tablet->tablet_id())) {
+ res = Status::Cancelled("tablet should do compaction locally");
+ break;
+ }
+ SingleReplicaCompaction single_compaction(_engine, tablet);
+ res = do_compact(single_compaction);
+ if (!res) {
+ LOG(WARNING) << "failed to do single replica compaction. res="
<< res
+ << ", table=" << tablet->tablet_id();
+ }
+ } while (0);
Review Comment:
Is there any special reason to use do {} while(0);
##########
be/src/olap/olap_common.h:
##########
@@ -49,7 +49,12 @@ using uint128_t = unsigned __int128;
using TabletUid = UniqueId;
-enum CompactionType { BASE_COMPACTION = 1, CUMULATIVE_COMPACTION = 2,
FULL_COMPACTION = 3 };
+enum CompactionType {
+ BASE_COMPACTION = 1,
+ CUMULATIVE_COMPACTION = 2,
+ FULL_COMPACTION = 3,
+ REMOTE_COMPACTION = 4
Review Comment:
should not add a new CompactionType, since it's not a new type but a new
policy under cumu.
##########
be/src/http/action/compaction_action.cpp:
##########
@@ -278,6 +279,23 @@ Status
CompactionAction::_execute_compaction_callback(TabletSharedPtr tablet,
<< ", table=" << tablet->tablet_id();
}
}
+ } else if (compaction_type == PARAM_COMPACTION_REMOTE) {
Review Comment:
I think it should be inside PARAM_COMPACTION_CUMULATIVE branch and and a
argument to indicate remote.
##########
be/src/http/action/compaction_action.cpp:
##########
@@ -243,7 +258,14 @@ Status
CompactionAction::_execute_compaction_callback(TabletSharedPtr tablet,
RETURN_IF_ERROR(compaction.prepare_compact());
return compaction.execute_compact();
};
- if (compaction_type == PARAM_COMPACTION_BASE) {
+ if (fetch_from_remote) {
+ SingleReplicaCompaction single_compaction(_engine, tablet);
+ res = do_compact(single_compaction);
Review Comment:
Should check if single_replica compaction enabled for this table.
##########
be/src/olap/single_replica_compaction.cpp:
##########
@@ -160,23 +153,19 @@ Status
SingleReplicaCompaction::_get_rowset_verisons_from_peer(
ExecEnv::GetInstance()->brpc_internal_client_cache()->get_client(addr.host,
addr.brpc_port);
if (stub == nullptr) {
- LOG(WARNING) << "get rowset versions from peer: get rpc stub failed,
host = " << addr.host
- << " port = " << addr.brpc_port;
- return Status::Cancelled("get rpc stub failed");
+ return Status::Aborted("get rpc stub failed");
}
brpc::Controller cntl;
stub->get_tablet_rowset_versions(&cntl, &request, &response, nullptr);
if (cntl.Failed()) {
- LOG(WARNING) << "open brpc connection to " << addr.host << " failed: "
<< cntl.ErrorText();
- return Status::Cancelled("open brpc connection failed");
+ return Status::Aborted("open brpc connection failed");
Review Comment:
What's the difference between Aborted and Cancelled?
##########
be/src/olap/single_replica_compaction.cpp:
##########
@@ -152,12 +149,10 @@ Status
SingleReplicaCompaction::_do_single_replica_compaction_impl() {
RETURN_IF_ERROR(modify_rowsets());
// 6. update last success compaction time
- if (compaction_type() == ReaderType::READER_CUMULATIVE_COMPACTION) {
Review Comment:
you can add set_last_single_compaction_success_time() and keep original ones.
##########
be/src/http/action/compaction_action.cpp:
##########
@@ -243,7 +258,14 @@ Status
CompactionAction::_execute_compaction_callback(TabletSharedPtr tablet,
RETURN_IF_ERROR(compaction.prepare_compact());
return compaction.execute_compact();
};
- if (compaction_type == PARAM_COMPACTION_BASE) {
+ if (fetch_from_remote) {
+ SingleReplicaCompaction single_compaction(_engine, tablet);
+ res = do_compact(single_compaction);
Review Comment:
check in do_compact.
##########
be/src/olap/single_replica_compaction.cpp:
##########
@@ -112,23 +110,18 @@ Status
SingleReplicaCompaction::_do_single_replica_compaction_impl() {
Version proper_version;
// 3. find proper version to fetch
if (!_find_rowset_to_fetch(peer_versions, &proper_version)) {
- LOG(WARNING) << _tablet->tablet_id() << " tablet don't need to fetch,
no matched version";
- return Status::Aborted("no matched version to fetch");
+ return Status::Cancelled("no matched versions for single replica
compaction");
Review Comment:
What's the difference between Aborted and Cancelled?
##########
be/src/olap/single_replica_compaction.h:
##########
@@ -30,20 +30,15 @@ class DataDir;
// SingleReplicaCompaction is used to fetch peer replica compaction result.
class SingleReplicaCompaction final : public CompactionMixin {
public:
- SingleReplicaCompaction(StorageEngine& engine, const TabletSharedPtr&
tablet,
- CompactionType compaction_type);
Review Comment:
I think it's not reasonable to remove compaction_type.
--
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
