Skip to content

branch-4.0: [improve](cloud) cloud reduce get_tablet_stats rpc to meta_service (#60543) - #61945

Merged
yiguolei merged 2 commits into
apache:branch-4.0from
mymeiyi:branch-4.0-pick-60543-1
Apr 15, 2026
Merged

branch-4.0: [improve](cloud) cloud reduce get_tablet_stats rpc to meta_service (#60543)#61945
yiguolei merged 2 commits into
apache:branch-4.0from
mymeiyi:branch-4.0-pick-60543-1

Conversation

@mymeiyi

Copy link
Copy Markdown
Contributor

pick cloud reduce get_tablet_stats rpc to meta_service (#60543)

@mymeiyi
mymeiyi requested a review from yiguolei as a code ownerMarch 31, 2026 10:11
CopilotAI review requested due to automatic review settings March 31, 2026 10:11
@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@mymeiyi

Copy link
Copy Markdown
ContributorAuthor

run buildall

@mymeiyimymeiyi changed the title [improve](cloud) cloud reduce get_tablet_stats rpc to meta_service (#60543)branch-4.0: [improve](cloud) cloud reduce get_tablet_stats rpc to meta_service (#60543)Mar 31, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes cloud-mode tablet statistics collection by reducing MetaService get_tablet_stats RPC volume, shifting to an “active tablets + interval ladder” fetch model, and introducing a master-to-follower stats sync RPC.

Changes:

  • Extend commit/compaction reporting to include tablet IDs so the FE can mark tablets “active” for stats refresh.
  • Add a new FE thrift RPC (syncCloudTabletStats) and implement master push + follower receive of tablet stats.
  • Introduce a versioned tablet-stats collection strategy (cloud_get_tablet_stats_version) with an interval ladder to reduce fetch frequency for stable tablets.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.

Show a summary per file
FileDescription
gensrc/thrift/FrontendService.thriftAdds tabletIds to commit report request and introduces syncCloudTabletStats RPC + request struct.
fe/fe-core/src/main/java/org/apache/doris/transaction/GlobalTransactionMgrIface.javaExtends afterCommitTxnResp to accept tabletIds.
fe/fe-core/src/main/java/org/apache/doris/transaction/GlobalTransactionMgr.javaUpdates interface implementation signature (no-op in non-cloud impl).
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.javaHandles tablet IDs in commit/compaction reports and implements syncCloudTabletStats.
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.javaAdds session variable cloud_force_sync_tablet_stats.
fe/fe-core/src/main/java/org/apache/doris/common/proc/TabletsProcDir.javaOptional “force sync” path to mark table tablets active when listing tablets (cloud-only, session-gated).
fe/fe-core/src/main/java/org/apache/doris/common/ClientPool.javaAdds frontendStatsPool for the new sync RPC.
fe/fe-core/src/main/java/org/apache/doris/cloud/transaction/CloudGlobalTransactionMgr.javaTracks committed tablet IDs and marks them active after commit.
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.javaAdds per-replica stats polling state (lastGetTabletStatsTime, statsIntervalIndex).
fe/fe-core/src/main/java/org/apache/doris/catalog/Replica.javaAdjusts default rowsetCount initialization (used by stats).
fe/fe-core/src/main/java/org/apache/doris/catalog/CloudTabletStatMgr.javaImplements versioned stats collection, active-tablet refresh, interval ladder, and master push to followers/observers.
fe/fe-core/src/main/java/org/apache/doris/alter/CloudSchemaChangeJobV2.javaMarks affected tablets active after schema change commit.
fe/fe-core/src/main/java/org/apache/doris/alter/CloudRollupJobV2.javaMarks affected tablets active after rollup creation.
fe/fe-common/src/main/java/org/apache/doris/common/Config.javaAdds cloud_get_tablet_stats_version and sync thread pool config.
be/src/cloud/cloud_meta_mgr.cppSends tablet IDs to FE in async commit report; uses txnId=-1 path for compaction-triggered stats refresh.
Comments suppressed due to low confidence (1)

fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:4574

  • On protobuf parse failure, this code prints a stack trace and still returns OK, which can silently drop stats updates. Please replace printStackTrace() with structured logging and return a non-OK TStatus (e.g., INVALID_ARGUMENT with an error message) so callers can detect/report the failure.
 } catch (InvalidProtocolBufferException e) {
// Handle the exception, log it, or take appropriate action
e.printStackTrace();
}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +565 to +567
TSyncCloudTabletStatsRequest request = new TSyncCloudTabletStatsRequest();
request.setTabletStatsPb(ByteBuffer.wrap(response.toByteArray()));
for (Frontend fe : frontends) {

CopilotAIMar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushTabletStats sets tabletStatsPb using ByteBuffer.wrap(...), but the receiving side (FrontendServiceImpl.syncCloudTabletStats) reads this field as a byte[] (request.getTabletStatsPb()). This looks like a type mismatch that will either not compile or will serialize incorrectly. Set the field using the expected byte[] payload (e.g., response.toByteArray()) and drop the ByteBuffer usage/import here.

Copilot uses AI. Check for mistakes.
Comment on lines +124 to +125
activeTablets.removeAll(copiedTablets);
getActiveTabletStats(copiedTablets);

CopilotAIMar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

activeTablets is cleared (removeAll) before getActiveTabletStats(copiedTablets) runs. If fetching tablet stats fails (RPC error / task failure), those tablet IDs are still removed and won’t be retried until they become active again, which can leave stats stale. Consider only removing IDs after a successful fetch, or re-adding failed IDs back to activeTablets.

Suggested change
activeTablets.removeAll(copiedTablets);
getActiveTabletStats(copiedTablets);
getActiveTabletStats(copiedTablets);
activeTablets.removeAll(copiedTablets);

Copilot uses AI. Check for mistakes.
return new TStatus(TStatusCode.INVALID_ARGUMENT);
List<Long> tabletIds = request.isSetTabletIds() ? request.getTabletIds() : Collections.emptyList();
if (LOG.isDebugEnabled()) {
LOG.debug("force sync tablet stats for txnId: {}, tabletNum: {}, tabletIds: {}", request.txnId,

CopilotAIMar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This debug log references request.txnId directly instead of using the Thrift accessor (getTxnId()), which is inconsistent with the rest of the method and can be misleading when the field is unset/defaulted. Prefer request.getTxnId() for consistency.

Suggested change
LOG.debug("force sync tablet stats for txnId: {}, tabletNum: {}, tabletIds: {}", request.txnId,
LOG.debug("force sync tablet stats for txnId: {}, tabletNum: {}, tabletIds: {}", request.getTxnId(),

Copilot uses AI. Check for mistakes.
Comment on lines +74 to +87
// last time to get tablet stats
@Getter
@Setter
long lastGetTabletStatsTime = 0;
/**
* The index of {@link org.apache.doris.catalog.CloudTabletStatMgr#DEFAULT_INTERVAL_LADDER_MS} array.
* Used to control the interval of getting tablet stats.
* When get tablet stats:
* if the stats is unchanged, will update this index to next value to get stats less frequently;
* if the stats is changed, will update this index to 0 to get stats more frequently.
*/
@Getter
@Setter
int statsIntervalIndex = 0;

CopilotAIMar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New fields lastGetTabletStatsTime and statsIntervalIndex are package-private and lack @SerializedName, while CloudReplica is Gson-persisted and other fields use short @SerializedName keys. Make these fields private and add appropriate @SerializedName (with alternates if needed) to keep metadata serialization stable/backward-compatible.

Copilot uses AI. Check for mistakes.
Comment on lines +78 to +84
/**
* The index of {@link org.apache.doris.catalog.CloudTabletStatMgr#DEFAULT_INTERVAL_LADDER_MS} array.
* Used to control the interval of getting tablet stats.
* When get tablet stats:
* if the stats is unchanged, will update this index to next value to get stats less frequently;
* if the stats is changed, will update this index to 0 to get stats more frequently.
*/

CopilotAIMar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadoc links to CloudTabletStatMgr#DEFAULT_INTERVAL_LADDER_MS, but that constant is private in CloudTabletStatMgr, so the Javadoc reference will be broken. Either make the constant accessible (public/protected) or remove/adjust the link text.

Copilot uses AI. Check for mistakes.
+ "Version 1: get all tablets; Version 2: get active and interval expired tablets"})
public static int cloud_get_tablet_stats_version = 2;

@ConfField(description = {"Maximum concurrent number of get tablet stat jobs."})

CopilotAIMar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config description for cloud_sync_tablet_stats_task_threads_num says "get tablet stat jobs", but this setting controls the concurrency of syncing/pushing tablet stats between FEs. Please update the description to match the actual behavior to avoid operator confusion.

Suggested change
@ConfField(description = {"Maximum concurrent number of get tablet stat jobs."})
@ConfField(description = {"存算分离模式下 FE 之间同步 tablet 统计信息任务的最大并发数。",
"Maximum concurrent number of syncing tablet stats between FEs."})

Copilot uses AI. Check for mistakes.
@doris-robot

Copy link
Copy Markdown

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage79.15% (1788/2259)
Line Coverage64.42% (31940/49580)
Region Coverage65.22% (15976/24494)
Branch Coverage55.81% (8501/15232)

@hello-stephen

Copy link
Copy Markdown
Contributor

FE UT Coverage Report

Increment line coverage 13.30% (31/233) 🎉
Increment coverage report
Complete coverage report

@hello-stephen

Copy link
Copy Markdown
Contributor

BE UT Coverage Report

Increment line coverage 0.00% (0/17) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage52.94% (19209/36284)
Line Coverage36.11% (178970/495572)
Region Coverage32.73% (138692/423769)
Branch Coverage33.70% (60268/178818)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 80.00% (12/15) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage71.31% (25331/35521)
Line Coverage54.00% (267119/494689)
Region Coverage51.64% (221027/428045)
Branch Coverage53.02% (95159/179461)

@mymeiyi

Copy link
Copy Markdown
ContributorAuthor

run buildall

@doris-robot

Copy link
Copy Markdown

Cloud UT Coverage Report

Increment line coverage 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage79.35% (1795/2262)
Line Coverage64.62% (32175/49794)
Region Coverage65.45% (16094/24590)
Branch Coverage56.03% (8579/15312)

@doris-robot

Copy link
Copy Markdown

BE UT Coverage Report

Increment line coverage 0.00% (0/17) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage52.90% (19233/36359)
Line Coverage36.09% (179138/496389)
Region Coverage32.71% (139034/425089)
Branch Coverage33.65% (60304/179194)

@hello-stephen

Copy link
Copy Markdown
Contributor

BE Regression && UT Coverage Report

Increment line coverage 58.82% (10/17) 🎉

Increment coverage report
Complete coverage report

CategoryCoverage
Function Coverage71.27% (25367/35593)
Line Coverage54.03% (267703/495495)
Region Coverage51.45% (220925/429362)
Branch Coverage52.94% (95205/179837)

@yiguolei
yiguolei merged commit 12ca10b into apache:branch-4.0Apr 15, 2026
24 of 28 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@mymeiyi@hello-stephen@doris-robot@yiguolei