[fix](cloud) refresh tablet meta of continuously ingested tablets - #67972
Merged
gavinchou merged 3 commits intoSep 16, 2026
Merged
Conversation
`sync_tablets()` selected the tablets it would work on by `last_sync_time_s` and then did two things to each: `sync_meta()` followed by `sync_rowsets()`. But `last_sync_time_s` only tracks how stale a tablet's ROWSETS are. It is advanced by `sync_tablet_rowsets()`, and only when that actually issues its RPC. A tablet under continuous ingest therefore keeps it permanently fresh, never falls below the staleness bound, and never has `sync_meta()` called on it at all. Its `TabletMeta` stays at whatever it was built with for the lifetime of the object, so properties that only arrive through the tablet meta -- `ttl_seconds` in particular -- are never picked up. An ALTER of the file cache TTL then has no effect on that tablet's newly written, read back or warmed up blocks, which keep computing their expiration from the stale value. Track meta staleness on its own clock. `last_sync_tablet_meta_time_s` is advanced only by `sync_meta()`, and `sync_tablets()` now decides per tablet which of the two RPCs it is due for instead of sorting tablets into a single bucket. A tablet that only needs its meta refreshed costs one RPC rather than the two a full sync costs. Pulling rowsets still implies pulling the meta, which is the relationship the single pass had: the rowsets are only as trustworthy as the meta they belong to. New bvars `sync_tablets_meta_num` and `sync_tablets_rowsets_num` split what `num_sync` used to lump together, and the finish log reports both. Test: a tablet whose rowset clock is fresh while its meta clock is stale is synced for meta only; one stale in both is synced for both; one fresh in both is skipped.
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
The name says "sync" while the clock only ever tracks how stale the tablet's rowsets are: sync_tablet_rowsets() is the sole writer, and sync_meta() now has a clock of its own next to it. Reading the old name as "when this tablet was last synced" is exactly the mistake that let meta work be gated on it. Mechanical rename, no behaviour change.
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Request changes. This was a static review of exact head 5a3de2f419a84ee389bbbc1c15f94680ee72f4e3 against base 17ac3d9275f8b7384c976d9bf8a5cc07512602b2. The intended separation of rowset and tablet-metadata freshness fixes the continuous-ingest starvation described by the PR, and the mechanical rename/reset coverage is complete, but the implementation is not safe to merge because of the five inline issues.
Critical checkpoint conclusions
- Goal and proof: The dispatch logic covers both stale, metadata-only stale, and both fresh; rowsets-only stale also maps to both RPCs as intended. The new unit test proves the first three selections, but it overwrites the natural post-load clocks, is single-threaded, changes no refreshed property, and does not exercise shared-schema behavior, so it does not prove the unsafe production cases are correct.
- Scope and parallel paths: All 12 authoritative changed paths were reviewed. The rename has no surviving
last_sync_time_sreference, every changed compaction reset still targets rowset freshness, and the load/query/compaction/internal-service refresh paths were traced. The patch is otherwise focused. - Concurrency and locking:
sync_tablets_thread, the internal-service light pool, query/load rowset refreshes, and the compaction producer/workers are genuinely concurrent. The two clocks have no common synchronization, and refreshable compaction metadata is written under_meta_lockbut read without that lock. The existing_sync_meta_lock -> _meta_lockorder is consistent and no new deadlock was found; it does not protect the scheduler or unlocked readers. - Lifecycle and ownership: Weak tablet ownership, eviction, stop-latch handling, and local static comparator initialization are sound. The process-global
TabletSchemaCacheownership is not:sync_meta()mutates a cache-deduplicated schema in place, leaking state across tablets and corrupting the cache key/value invariant. No new cross-TU static-order issue was found. - Conditions and errors: The four freshness conditions are understandable, but queued decisions are not revalidated after concurrent refreshes. Statuses are checked; metadata
NOT_FOUNDclears/skips the tablet, rowsetNOT_FOUNDalso clears it, and continuing rowset refresh after other metadata errors is inherited and safe because the old valid metadata remains and rowset responses carry their own schema/counter data. - Configuration, compatibility, persistence, and writes: No new configuration, wire/storage format, FE/BE variable, transaction, persistence, or direct data-write surface is introduced. The existing dynamic interval is sampled on every pass, and no rolling-upgrade issue was found.
- Observability and performance: The split bvars and finish log adequately expose attempted metadata versus rowset work. However, zero-initialized metadata clocks create a redundant post-load GetTablet wave and stale queued flags create further obsolete RPCs; those are reported inline.
- Tests and CI: Per the review contract I did not build, format, or run tests. GitHub currently reports BE UT, compile, formatter, P0, non-concurrent, cloud P0, vault P0, and coverage success; External Regression and performance report failures, which this static review did not attribute. The release note matches the intended user-visible fix.
- User focus and completion: No extra user focus was provided. Three bounded review rounds completed; the final normal and risk-focused reviewers all returned
NO_NEW_VALUABLE_FINDINGS, every candidate was adjudicated, and the review is complete rather than capped/incomplete.
gavinchou
approved these changes
Sep 16, 2026
yiguolei
pushed a commit
that referenced
this pull request
Sep 16, 2026
yiguolei
pushed a commit
that referenced
this pull request
Sep 17, 2026
morningman
added a commit
that referenced
this pull request
Sep 18, 2026
…on PRs from master in merge order (#67783 #68034 #68033 #68057 #68094 #68087) (#68151) Cherry-picked from #67783, #68034, #68033, #68057, #68094, #68087 Batch pick of every merged PR carrying the `incremental-computation` label that `branch-incremental-computation` does not have yet (no `incremental-computation-picked` label), in the order they landed on master (`git log --first-parent`). One commit per PR, each created with `git cherry-pick -x` so the message ends with `(cherry picked from commit <master sha>)`. Follows the same convention as #67830, #68017 and #68073. | # | Master commit | PR | Title | |---|---|---|---| | 1 | e545b13 | #67783 | [fix](policy) Enforce row policies on MOW time travel | | 2 | f7a0842 | #68034 | [fix](binlog) Fix missing DELETE events in row binlog | | 3 | e85575e | #68033 | [fix](binlog) Preserve row binlog compaction policy | | 4 | 21160d7 | #68057 | [fix](binlog) Decouple row binlog compaction from CCR binlog config | | 5 | 04aa5a1 | #68094 | [fix](binlog) Persist row binlog config updates in cloud mode | | 6 | da8feed | #68087 | [fix](binlog) Report streams with missing base tables as stale | Not included on purpose: - The 20 labelled PRs that already carry `incremental-computation-picked` (#62606 in the fork point, #67508 via #67712, the nine of #67830, the six of #68017, the two of #68073, and #68050 whose content this branch got directly through #68012). - #68012 carries the label but is a PR against this branch itself (merged as `6f7c87fa892`); nothing to pick. - #67820 is still open on master; this branch already carries its content via #67861. ### Prerequisite check For every pick I listed the master commits between the fork point (`efedf10c7e3`) and the pick that touch the same files and are not on this branch, and checked whether the pick's behavior depends on them. - **#67783** declares no related PR. It builds on the MOW time-travel rewrite (`BindRelation.buildMowTimeTravelUnion`, #67480) which is already here. Two unlabelled master commits overlap: #66770 (authorization plugin SPI: reworks the data-mask / row-filter API of `LogicalCheckPolicy` and its test) and #67811 (removes the `isPlayNereidsDump()` check in `CheckPolicy`). Neither is a functional prerequisite: the fix consists of `getPolicyTable()` unwrapping `OlapTableWrapper`, `CheckPolicy` collecting the whole filter chain below the policy mask, and `BindRelation` putting a `LogicalCheckPolicy` above each union branch — none of that uses the #66770 API. Not picked; adapted instead (see below). - **#68034**, **#68033**, **#68087** (#68087 relates to #67173, which is before the fork point): no unpicked master commit touches any of their files; the picks applied cleanly and are byte-identical to master. - **#68057**: only `be/test/cloud/cloud_compaction_test.cpp` overlaps with the unrelated #67972 (refresh tablet meta of continuously ingested tablets); auto-merged, the pick only adds new `TEST_F` blocks. Main-code hunks are byte-identical to master. - **#68094**: overlaps with #68090 / #67972 / #66598 (`cloud_tablet.cpp`), #67295 / #67618 / #68090 (`base_tablet.{h,cpp}`), #66598 / #67637 (`meta_service.cpp`), #66598 (`cloud.proto`, `cloud_tablet_test.cpp`) and #67761 / #66598 / #67637 (`meta_service_test.cpp`). All auto-merged. The fix — `BaseTablet::binlog_config()` under the meta lock, `BinlogConfig::operator==`, `CloudTablet::sync_meta()` refreshing the binlog config, `update_tablet` in the meta service accepting `binlog_config`, and `CloudSchemaChangeHandler` routing ROW-binlog property updates through it — does not use anything those commits add. Its hunks are byte-identical to master except one trailing context line in `sync_meta()` (`last_sync_tablet_meta_time_s` comes from #67972). Only #67783 needed adaptation, recorded in its commit message: - `LogicalCheckPolicy.java`: the conflicting context was master's `parsePolicyExpression()` helper (#66770), which does not exist here; the new `getPolicyTable()` is inserted in the same place without it. - `CheckRowPolicyTest.java`: the data-mask mock is written against this branch's per-column `AccessControllerManager.evalDataMaskPolicy(..., column) -> Optional<DataMaskPolicy>` instead of master's `evalDataMaskPolicies(..., Set<String>) -> Map<String, DataMaskSpec>`. The masks are the same (concat for the random-distribution table; the non-movable `k2` mask plus identity masks for the hidden reconstruction columns of the MOW table). The master-only `Or` import (#66770) is not carried; `Collections` / `Locale` imports were added because they arrive with #66770 on master. ### Drift check against master Each pick's `+`/`-` lines are identical to the master commit's, except for the #67783 adaptation above. After the six picks, the touched files still differ from master at `da8feed859d` in: `cloud_tablet.cpp`, `base_tablet.{h,cpp}`, `cloud_compaction_test.cpp`, `cloud_tablet_test.cpp`, `meta_service.cpp`, `meta_service_test.cpp`, `cloud.proto`, `CheckPolicy.java`, `LogicalCheckPolicy.java`, `CheckRowPolicyTest.java`. Replaying the nine unpicked master commits listed above (#67637, #67618, #66770, #66598, #67972, #67811, #67761, #67295, #68090) in a temporary index and removing this branch's own #67861 (`GetTsoRecoveryTransactions` in `cloud.proto` / `meta_service_test.cpp`) brings every file to zero diff against master, except the two `LogicalCheckPolicy.java` / `CheckRowPolicyTest.java` hunks of #66770 that overlap the adaptation. Nothing else is left over. ### Verification - FE: `run-fe-ut.sh --run` on this branch (regenerates thrift/protobuf, compiles fe-core main + test) with the test classes touched by the picks plus `DeleteFromCommandTest` from #68034's checklist: 5 classes, 58 tests, 0 failures, 0 errors, BUILD SUCCESS — `CheckRowPolicyTest` 9 (the three tests #67783 adds included), `ExplainTableStreamPlanTest` 24, `CloudSchemaChangeHandlerTest` 18 (the tests of #68033 and #68094 included), `DropTableStreamTest` 5, `DeleteFromCommandTest` 2. - FE checkstyle on fe-core: 0 violations. - BE: `-fsyntax-only` with the flags of the Release build (`compile_commands.json`, regenerated `gen_cpp` headers incl. the new `TabletMetaInfoPB.binlog_config`) passes for `cloud/cloud_tablet.cpp`, `cloud/cloud_storage_engine.cpp`, `storage/olap_server.cpp`, `storage/tablet/tablet.cpp`, `storage/tablet/base_tablet.cpp`, and with `-DBE_TEST -fno-access-control` for `test/cloud/cloud_compaction_test.cpp`, `test/cloud/cloud_tablet_test.cpp`, `test/storage/compaction/compaction_task_test.cpp`, `test/storage/tablet/tablet_test.cpp`. - Meta service: the two-line `update_tablet` hunk of #68094 only uses the generated `TabletMetaInfoPB::has_binlog_config()/binlog_config()` and `TabletMetaCloudPB::mutable_binlog_config()` accessors, all present in the regenerated headers. - The two new groovy suites (`row_binlog_p0/test_row_binlog_mow_light_delete`, `time_travel_p0/test_mow_time_travel_row_policy`) parse cleanly (groovy parser check). --------- Co-authored-by: morrySnow <zhangwenxin@selectdb.com> Co-authored-by: Luwei <814383175@qq.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
CloudTabletMgr::sync_tablets()selected the tablets it would work on bylast_sync_time_s, then did two things to each one:sync_meta()followed bysync_rowsets().But that clock only tracks how stale a tablet's rowsets are. It is advanced inside
sync_tablet_rowsets(), and only when that actually issues its RPC. A tablet under continuous ingest therefore keeps it permanently fresh, never falls below the staleness bound, and never hassync_meta()called on it at all.Its
TabletMetathen stays at whatever it was built with for the lifetime of the object, so properties that only arrive through the tablet meta are never picked up.ttl_secondsis the one that shows:CloudTablet::sync_meta()is the only thing that refreshes it, and it feeds the file cache expiration computed on the write path (CloudRowsetBuilder, compaction output), the read path (TabletReader,OlapScanner) and warm-up. AnALTER TABLE ... SET ("file_cache_ttl_seconds" = ...)on a table under load therefore has no effect on those blocks, which keep landing in the wrong queue with an expiration derived from the stale value.Release note
Fixed tablet metadata never being refreshed for tablets under continuous ingest, which left properties such as
file_cache_ttl_secondsstale on those tablets after an ALTER.What is changed and how it works?
1. Give meta staleness its own clock.
last_sync_tablet_meta_time_sis advanced only bysync_meta().2. Rename
last_sync_time_stolast_sync_rowsets_time_s(separate commit, mechanical). The old name says "sync" while the clock only ever tracked rowsets, and reading it as "when this tablet was last synced" is exactly the mistake that let meta work be gated on it. With a second clock alongside it the old name would be actively misleading. This also keeps the field named the same as on branch-3.1.3.
sync_tablets()decides per tablet which of the two RPCs it is due for, rather than sorting tablets into a single bucket:Work is still ordered by the older of the two clocks, so a mid-run stop has already served the tablets that waited longest.
New bvars
sync_tablets_meta_numandsync_tablets_rowsets_numsplit whatnum_syncused to lump together, and the finish log reports both.Check List (For Author)
CloudTabletMgrTest.SyncTabletsRefreshesMetaOfContinuouslyIngestedTabletcovers all three cases: a tablet whose rowset clock is fresh while its meta clock is stale is synced for meta only, one stale in both is synced for both, one fresh in both is skipped.DORIS_TOOLCHAIN=clang DISABLE_BE_JAVA_EXTENSIONS=ON ENABLE_INJECTION_POINT=ON ENABLE_PCH=0 sh run-be-ut.sh --run --filter='CloudTabletMgrTest.*'This is a refactor/code format and no logic has been changed.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)