Copilot commented on code in PR #67716:
URL: https://github.com/apache/doris/pull/67716#discussion_r3966308915
##########
cloud/src/meta-service/meta_service_txn.cpp:
##########
@@ -4658,6 +4658,16 @@ void MetaServiceImpl::get_prepare_txn_by_coordinator(
do {
err = txn->get(begin_info_key, end_info_key, &it, true);
+ TEST_SYNC_POINT_CALLBACK("get_prepare_txn_by_coordinator::range_get",
&err);
Review Comment:
`TEST_SYNC_POINT_CALLBACK` is executed on every scan iteration in a
production code path. If this macro is not fully compiled out in non-test
builds, it can add overhead. Consider guarding it so it only exists in
test/debug builds (or ensure the macro is a true no-op with zero runtime cost
in release).
##########
cloud/test/meta_service_test.cpp:
##########
@@ -2926,6 +2926,45 @@ TEST(MetaServiceTest, GetPrepareTxnByCoordinatorTest) {
ASSERT_EQ(resp.txn_infos_size(), 5);
}
+ // Resume an expired scan at the failed page without losing or duplicating
transactions.
+ for (int failed_page : {1, 3, 5, 6}) {
+ auto sp = SyncPoint::get_instance();
+ DORIS_CLOUD_DEFER {
+ sp->disable_processing();
+ sp->clear_all_call_backs();
+ };
+ int pages = 0;
+ int reads = 0;
+ sp->set_call_back("memkv::Transaction::get", [&](auto&& args) {
+ *try_any_cast<int*>(args[0]) = 1;
+ ++reads;
+ });
+ sp->set_call_back("get_prepare_txn_by_coordinator::range_get",
[&](auto&& args) {
+ if (++pages == failed_page) {
+ *try_any_cast<TxnErrorCode*>(args[0]) =
TxnErrorCode::TXN_TOO_OLD;
+ }
+ });
+ sp->enable_processing();
+
+ brpc::Controller cntl;
+ GetPrepareTxnByCoordinatorRequest req;
+ GetPrepareTxnByCoordinatorResponse resp;
+ req.set_cloud_unique_id(cloud_unique_id);
+ req.set_id(coordinator_id);
+ req.set_ip(host);
+ meta_service->get_prepare_txn_by_coordinator(
+ reinterpret_cast<::google::protobuf::RpcController*>(&cntl),
&req, &resp, nullptr);
+
+ ASSERT_EQ(resp.status().code(), MetaServiceCode::OK);
+ // MemTxnKv needs an empty page after the five full pages to finish
the scan.
+ ASSERT_EQ(pages, 6);
+ ASSERT_EQ(reads, 7);
Review Comment:
These assertions bake in internal paging behavior (`pages == 6`, `reads ==
7`) that’s likely to change with page size, iterator behavior, or
implementation tweaks, making the test brittle. Prefer asserting the observable
contract (no loss/duplication, full coverage, success) and/or deriving
expectations from known constants (e.g., page size / `txn_ids.size()`) rather
than hard-coded magic numbers.
##########
cloud/src/meta-service/meta_service_txn.cpp:
##########
@@ -4658,6 +4658,16 @@ void MetaServiceImpl::get_prepare_txn_by_coordinator(
do {
err = txn->get(begin_info_key, end_info_key, &it, true);
+ TEST_SYNC_POINT_CALLBACK("get_prepare_txn_by_coordinator::range_get",
&err);
+ if (err == TxnErrorCode::TXN_TOO_OLD) {
+ err = txn_kv_->create_txn(&txn);
+ if (err != TxnErrorCode::TXN_OK) {
+ msg = "failed to create txn";
Review Comment:
The new error message `"failed to create txn"` loses the underlying reason
(`err`) and the context (retry after `TXN_TOO_OLD`). Include `err` (and ideally
that this happened during the `TXN_TOO_OLD` retry path) so logs/debugging can
distinguish initial failures from retry failures.
##########
cloud/test/meta_service_test.cpp:
##########
@@ -2926,6 +2926,45 @@ TEST(MetaServiceTest, GetPrepareTxnByCoordinatorTest) {
ASSERT_EQ(resp.txn_infos_size(), 5);
}
+ // Resume an expired scan at the failed page without losing or duplicating
transactions.
+ for (int failed_page : {1, 3, 5, 6}) {
+ auto sp = SyncPoint::get_instance();
+ DORIS_CLOUD_DEFER {
+ sp->disable_processing();
+ sp->clear_all_call_backs();
+ };
+ int pages = 0;
+ int reads = 0;
+ sp->set_call_back("memkv::Transaction::get", [&](auto&& args) {
+ *try_any_cast<int*>(args[0]) = 1;
+ ++reads;
+ });
+ sp->set_call_back("get_prepare_txn_by_coordinator::range_get",
[&](auto&& args) {
+ if (++pages == failed_page) {
+ *try_any_cast<TxnErrorCode*>(args[0]) =
TxnErrorCode::TXN_TOO_OLD;
+ }
+ });
+ sp->enable_processing();
+
+ brpc::Controller cntl;
+ GetPrepareTxnByCoordinatorRequest req;
+ GetPrepareTxnByCoordinatorResponse resp;
+ req.set_cloud_unique_id(cloud_unique_id);
+ req.set_id(coordinator_id);
+ req.set_ip(host);
+ meta_service->get_prepare_txn_by_coordinator(
+ reinterpret_cast<::google::protobuf::RpcController*>(&cntl),
&req, &resp, nullptr);
+
+ ASSERT_EQ(resp.status().code(), MetaServiceCode::OK);
+ // MemTxnKv needs an empty page after the five full pages to finish
the scan.
+ ASSERT_EQ(pages, 6);
+ ASSERT_EQ(reads, 7);
+ ASSERT_EQ(resp.txn_infos_size(), txn_ids.size());
+ for (int i = 0; i < resp.txn_infos_size(); ++i) {
+ EXPECT_EQ(resp.txn_infos(i).txn_id(), txn_ids[i]);
+ }
Review Comment:
This assumes the response order is exactly the same as `txn_ids`. If the API
contract does not explicitly guarantee ordering, this can cause flaky failures.
Consider comparing as sets (or sorting both lists before comparing) so the test
validates “same elements, no duplicates/missing” without depending on ordering.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
