github-actions[bot] commented on code in PR #67972:
URL: https://github.com/apache/doris/pull/67972#discussion_r4016032967
##########
be/src/cloud/cloud_tablet.h:
##########
@@ -348,7 +348,14 @@ class CloudTablet final : public BaseTablet {
const auto& rowset_map() const { return _rs_version_map; }
- int64_t last_sync_time_s = 0;
+ // How long since this tablet's ROWSETS were pulled from MS. Only
sync_rowsets() advances
+ // it, and only when it actually issues the RPC.
+ int64_t last_sync_rowsets_time_s = 0;
+ // How long since this tablet's META was pulled from MS, which is what
carries properties
+ // such as the file cache TTL. Only sync_meta() advances it. Tracked
separately on
+ // purpose: a tablet under continuous ingest keeps the rowsets clock
permanently fresh,
+ // so gating meta work on that one starves the meta refresh entirely.
+ int64_t last_sync_tablet_meta_time_s = 0;
Review Comment:
[P1] Synchronize the new metadata refresh clock
The dedicated `sync_tablets_thread` reads this field in
`CloudTabletMgr::sync_tablets()`, while
`CloudInternalServiceImpl::sync_tablet_meta()` can run `sync_meta()`
concurrently in `_light_work_pool` and write it. The writer holds
`_sync_meta_lock`, but the scheduler does not, so this new state has a C++ data
race and its due/not-due decision is undefined. Please make both scheduling
clocks atomic (relaxed loads/stores are sufficient if they remain timestamp
hints) or guard every scheduler/RPC/compaction access with one documented
common lock.
##########
be/src/cloud/cloud_tablet_mgr.cpp:
##########
@@ -386,53 +391,93 @@ std::vector<std::weak_ptr<CloudTablet>>
CloudTabletMgr::get_weak_tablets() {
void CloudTabletMgr::sync_tablets(const CountDownLatch& stop_latch) {
LOG_INFO("begin to sync tablets");
- int64_t last_sync_time_bound = ::time(nullptr) -
config::tablet_sync_interval_s;
- auto weak_tablets = get_weak_tablets();
+ // A tablet carries two staleness clocks and each one gates a different
RPC:
+ //
+ // last_sync_rowsets_time_s
+ // how long since we pulled this tablet's ROWSETS from MS. Only
sync_rowsets()
+ // advances it, and only when it actually issues the RPC -- a query
whose requested
+ // version we already hold returns early and leaves the clock
untouched.
+ //
+ // last_sync_tablet_meta_time_s
+ // how long since we pulled this tablet's META from MS, which is
what carries
+ // properties such as the file cache TTL. Only sync_meta() advances
it.
+ //
+ // They have to be read separately. A tablet under continuous ingest keeps
the rowsets
+ // clock permanently fresh, so selecting meta work by it -- as this used
to -- means such a
+ // tablet never has its meta refreshed at all, and it keeps serving
whatever TTL it was
+ // built with.
+ const int64_t stale_before = ::time(nullptr) -
config::tablet_sync_interval_s;
+
+ struct Work {
+ std::weak_ptr<CloudTablet> tablet;
+ bool needs_meta = false;
+ bool needs_rowsets = false;
+ };
- // sort by last_sync_time
+ // Ordered by the older of the two clocks, so that if we are told to stop
half way, the
+ // tablets that have been waiting longest have already been served.
static auto cmp = [](const auto& a, const auto& b) { return a.first <
b.first; };
- std::multiset<std::pair<int64_t, std::weak_ptr<CloudTablet>>,
decltype(cmp)>
- sync_time_tablet_set(cmp);
+ std::multiset<std::pair<int64_t, Work>, decltype(cmp)> due(cmp);
- for (auto& weak_tablet : weak_tablets) {
- if (auto tablet = weak_tablet.lock()) {
- int64_t last_sync_time = tablet->last_sync_time_s;
- if (last_sync_time <= last_sync_time_bound) {
- sync_time_tablet_set.emplace(last_sync_time, weak_tablet);
- }
+ for (auto& weak_tablet : get_weak_tablets()) {
+ auto tablet = weak_tablet.lock();
+ if (!tablet) {
+ continue;
+ }
+ const bool needs_rowsets = tablet->last_sync_rowsets_time_s <=
stale_before;
+ Work work {
+ .tablet = weak_tablet,
+ // Pulling rowsets implies pulling the tablet meta: the
rowsets we are about
+ // to take are only as trustworthy as the meta they belong to,
and this is
+ // the relationship the previous single pass had.
+ .needs_meta = needs_rowsets ||
tablet->last_sync_tablet_meta_time_s <= stale_before,
Review Comment:
[P1] Seed the metadata clock after the cache-miss fetch
A normal cache miss has just completed `get_tablet_meta()` before
constructing this tablet, but the new clock remains zero; the initial rowset
sync only advances the rowset clock. Consequently the next scheduler pass (600
seconds by default) treats every newly cached tablet as metadata-stale instead
of honoring the 1,800-second interval, adding one unnecessary GetTablet RPC per
cached tablet after restart/cache churn (up to the default 100,000-tablet
capacity). Stamp the clock when the fetched metadata is installed in
`CloudTabletMgr::get_tablet()` and add a test that leaves the post-load clocks
untouched.
##########
be/src/cloud/cloud_tablet_mgr.cpp:
##########
@@ -386,53 +391,93 @@ std::vector<std::weak_ptr<CloudTablet>>
CloudTabletMgr::get_weak_tablets() {
void CloudTabletMgr::sync_tablets(const CountDownLatch& stop_latch) {
LOG_INFO("begin to sync tablets");
- int64_t last_sync_time_bound = ::time(nullptr) -
config::tablet_sync_interval_s;
- auto weak_tablets = get_weak_tablets();
+ // A tablet carries two staleness clocks and each one gates a different
RPC:
+ //
+ // last_sync_rowsets_time_s
+ // how long since we pulled this tablet's ROWSETS from MS. Only
sync_rowsets()
+ // advances it, and only when it actually issues the RPC -- a query
whose requested
+ // version we already hold returns early and leaves the clock
untouched.
+ //
+ // last_sync_tablet_meta_time_s
+ // how long since we pulled this tablet's META from MS, which is
what carries
+ // properties such as the file cache TTL. Only sync_meta() advances
it.
+ //
+ // They have to be read separately. A tablet under continuous ingest keeps
the rowsets
+ // clock permanently fresh, so selecting meta work by it -- as this used
to -- means such a
+ // tablet never has its meta refreshed at all, and it keeps serving
whatever TTL it was
+ // built with.
+ const int64_t stale_before = ::time(nullptr) -
config::tablet_sync_interval_s;
+
+ struct Work {
+ std::weak_ptr<CloudTablet> tablet;
+ bool needs_meta = false;
+ bool needs_rowsets = false;
+ };
- // sort by last_sync_time
+ // Ordered by the older of the two clocks, so that if we are told to stop
half way, the
+ // tablets that have been waiting longest have already been served.
static auto cmp = [](const auto& a, const auto& b) { return a.first <
b.first; };
- std::multiset<std::pair<int64_t, std::weak_ptr<CloudTablet>>,
decltype(cmp)>
- sync_time_tablet_set(cmp);
+ std::multiset<std::pair<int64_t, Work>, decltype(cmp)> due(cmp);
- for (auto& weak_tablet : weak_tablets) {
- if (auto tablet = weak_tablet.lock()) {
- int64_t last_sync_time = tablet->last_sync_time_s;
- if (last_sync_time <= last_sync_time_bound) {
- sync_time_tablet_set.emplace(last_sync_time, weak_tablet);
- }
+ for (auto& weak_tablet : get_weak_tablets()) {
+ auto tablet = weak_tablet.lock();
+ if (!tablet) {
+ continue;
+ }
+ const bool needs_rowsets = tablet->last_sync_rowsets_time_s <=
stale_before;
+ Work work {
+ .tablet = weak_tablet,
+ // Pulling rowsets implies pulling the tablet meta: the
rowsets we are about
+ // to take are only as trustworthy as the meta they belong to,
and this is
+ // the relationship the previous single pass had.
+ .needs_meta = needs_rowsets ||
tablet->last_sync_tablet_meta_time_s <= stale_before,
+ .needs_rowsets = needs_rowsets};
+ if (!work.needs_meta && !work.needs_rowsets) {
+ continue;
}
+ due.emplace(
+ std::min(tablet->last_sync_tablet_meta_time_s,
tablet->last_sync_rowsets_time_s),
+ std::move(work));
}
int num_sync = 0;
- for (auto&& [_, weak_tablet] : sync_time_tablet_set) {
+ int num_sync_meta = 0;
+ for (auto&& [_, work] : due) {
Review Comment:
[P2] Revalidate queued refresh work before issuing RPCs
These booleans were captured while the entire due set was built, and the
loop can wait behind many earlier tablets. During that gap a
load/query/compaction path can refresh rowsets, or the internal-service worker
can refresh metadata. The old code re-read the rowset clock immediately before
work; now `sync_meta()` and `sync_rowsets(query_version=-1)` will issue the
same RPC again after waiting for the concurrent refresh. Re-evaluate each clock
at execution time (ideally inside the serialized operation to close the wait
window) so a large pass does not create O(resident-tablets) obsolete MS calls.
##########
be/src/cloud/cloud_tablet_mgr.cpp:
##########
@@ -386,53 +391,93 @@ std::vector<std::weak_ptr<CloudTablet>>
CloudTabletMgr::get_weak_tablets() {
void CloudTabletMgr::sync_tablets(const CountDownLatch& stop_latch) {
LOG_INFO("begin to sync tablets");
- int64_t last_sync_time_bound = ::time(nullptr) -
config::tablet_sync_interval_s;
- auto weak_tablets = get_weak_tablets();
+ // A tablet carries two staleness clocks and each one gates a different
RPC:
+ //
+ // last_sync_rowsets_time_s
+ // how long since we pulled this tablet's ROWSETS from MS. Only
sync_rowsets()
+ // advances it, and only when it actually issues the RPC -- a query
whose requested
+ // version we already hold returns early and leaves the clock
untouched.
+ //
+ // last_sync_tablet_meta_time_s
+ // how long since we pulled this tablet's META from MS, which is
what carries
+ // properties such as the file cache TTL. Only sync_meta() advances
it.
+ //
+ // They have to be read separately. A tablet under continuous ingest keeps
the rowsets
+ // clock permanently fresh, so selecting meta work by it -- as this used
to -- means such a
+ // tablet never has its meta refreshed at all, and it keeps serving
whatever TTL it was
+ // built with.
+ const int64_t stale_before = ::time(nullptr) -
config::tablet_sync_interval_s;
+
+ struct Work {
+ std::weak_ptr<CloudTablet> tablet;
+ bool needs_meta = false;
+ bool needs_rowsets = false;
+ };
- // sort by last_sync_time
+ // Ordered by the older of the two clocks, so that if we are told to stop
half way, the
+ // tablets that have been waiting longest have already been served.
static auto cmp = [](const auto& a, const auto& b) { return a.first <
b.first; };
- std::multiset<std::pair<int64_t, std::weak_ptr<CloudTablet>>,
decltype(cmp)>
- sync_time_tablet_set(cmp);
+ std::multiset<std::pair<int64_t, Work>, decltype(cmp)> due(cmp);
- for (auto& weak_tablet : weak_tablets) {
- if (auto tablet = weak_tablet.lock()) {
- int64_t last_sync_time = tablet->last_sync_time_s;
- if (last_sync_time <= last_sync_time_bound) {
- sync_time_tablet_set.emplace(last_sync_time, weak_tablet);
- }
+ for (auto& weak_tablet : get_weak_tablets()) {
+ auto tablet = weak_tablet.lock();
+ if (!tablet) {
+ continue;
+ }
+ const bool needs_rowsets = tablet->last_sync_rowsets_time_s <=
stale_before;
+ Work work {
+ .tablet = weak_tablet,
+ // Pulling rowsets implies pulling the tablet meta: the
rowsets we are about
+ // to take are only as trustworthy as the meta they belong to,
and this is
+ // the relationship the previous single pass had.
+ .needs_meta = needs_rowsets ||
tablet->last_sync_tablet_meta_time_s <= stale_before,
+ .needs_rowsets = needs_rowsets};
+ if (!work.needs_meta && !work.needs_rowsets) {
+ continue;
}
+ due.emplace(
+ std::min(tablet->last_sync_tablet_meta_time_s,
tablet->last_sync_rowsets_time_s),
+ std::move(work));
}
int num_sync = 0;
- for (auto&& [_, weak_tablet] : sync_time_tablet_set) {
+ int num_sync_meta = 0;
+ for (auto&& [_, work] : due) {
if (stop_latch.count() <= 0) {
break;
}
+ auto tablet = work.tablet.lock();
+ if (!tablet) {
+ continue;
+ }
- if (auto tablet = weak_tablet.lock()) {
- if (tablet->last_sync_time_s > last_sync_time_bound) {
- continue;
- }
-
- ++num_sync;
+ if (work.needs_meta) {
Review Comment:
[P1] Synchronize metadata readers before broadening periodic refresh
This new periodic branch calls `sync_meta()` for actively ingested tablets
while the separate compaction-producer thread is selecting work. `sync_meta()`
writes `TabletMeta::_compaction_policy` (a `std::string`), the time-series
thresholds, vertical-group size, and `TabletSchema::_disable_auto_compaction`
under `_meta_lock`, but `get_topn_tablets_to_compact()` and
`get_cloud_base_compaction_score()` read those fields without that lock. The
resulting string/scalar read-write races are undefined behavior and can corrupt
or mis-schedule compaction. The existing best-effort push already exposed the
unsafe contract, but this PR adds the periodic writer path for the previously
skipped population; please give every refreshable property a consistent
lock-taking/snapshot accessor contract before enabling it.
##########
be/src/cloud/cloud_tablet_mgr.cpp:
##########
@@ -386,53 +391,93 @@ std::vector<std::weak_ptr<CloudTablet>>
CloudTabletMgr::get_weak_tablets() {
void CloudTabletMgr::sync_tablets(const CountDownLatch& stop_latch) {
LOG_INFO("begin to sync tablets");
- int64_t last_sync_time_bound = ::time(nullptr) -
config::tablet_sync_interval_s;
- auto weak_tablets = get_weak_tablets();
+ // A tablet carries two staleness clocks and each one gates a different
RPC:
+ //
+ // last_sync_rowsets_time_s
+ // how long since we pulled this tablet's ROWSETS from MS. Only
sync_rowsets()
+ // advances it, and only when it actually issues the RPC -- a query
whose requested
+ // version we already hold returns early and leaves the clock
untouched.
+ //
+ // last_sync_tablet_meta_time_s
+ // how long since we pulled this tablet's META from MS, which is
what carries
+ // properties such as the file cache TTL. Only sync_meta() advances
it.
+ //
+ // They have to be read separately. A tablet under continuous ingest keeps
the rowsets
+ // clock permanently fresh, so selecting meta work by it -- as this used
to -- means such a
+ // tablet never has its meta refreshed at all, and it keeps serving
whatever TTL it was
+ // built with.
+ const int64_t stale_before = ::time(nullptr) -
config::tablet_sync_interval_s;
+
+ struct Work {
+ std::weak_ptr<CloudTablet> tablet;
+ bool needs_meta = false;
+ bool needs_rowsets = false;
+ };
- // sort by last_sync_time
+ // Ordered by the older of the two clocks, so that if we are told to stop
half way, the
+ // tablets that have been waiting longest have already been served.
static auto cmp = [](const auto& a, const auto& b) { return a.first <
b.first; };
- std::multiset<std::pair<int64_t, std::weak_ptr<CloudTablet>>,
decltype(cmp)>
- sync_time_tablet_set(cmp);
+ std::multiset<std::pair<int64_t, Work>, decltype(cmp)> due(cmp);
- for (auto& weak_tablet : weak_tablets) {
- if (auto tablet = weak_tablet.lock()) {
- int64_t last_sync_time = tablet->last_sync_time_s;
- if (last_sync_time <= last_sync_time_bound) {
- sync_time_tablet_set.emplace(last_sync_time, weak_tablet);
- }
+ for (auto& weak_tablet : get_weak_tablets()) {
+ auto tablet = weak_tablet.lock();
+ if (!tablet) {
+ continue;
+ }
+ const bool needs_rowsets = tablet->last_sync_rowsets_time_s <=
stale_before;
+ Work work {
+ .tablet = weak_tablet,
+ // Pulling rowsets implies pulling the tablet meta: the
rowsets we are about
+ // to take are only as trustworthy as the meta they belong to,
and this is
+ // the relationship the previous single pass had.
+ .needs_meta = needs_rowsets ||
tablet->last_sync_tablet_meta_time_s <= stale_before,
+ .needs_rowsets = needs_rowsets};
+ if (!work.needs_meta && !work.needs_rowsets) {
+ continue;
}
+ due.emplace(
+ std::min(tablet->last_sync_tablet_meta_time_s,
tablet->last_sync_rowsets_time_s),
+ std::move(work));
}
int num_sync = 0;
- for (auto&& [_, weak_tablet] : sync_time_tablet_set) {
+ int num_sync_meta = 0;
+ for (auto&& [_, work] : due) {
if (stop_latch.count() <= 0) {
break;
}
+ auto tablet = work.tablet.lock();
+ if (!tablet) {
+ continue;
+ }
- if (auto tablet = weak_tablet.lock()) {
- if (tablet->last_sync_time_s > last_sync_time_bound) {
- continue;
- }
-
- ++num_sync;
+ if (work.needs_meta) {
+ ++num_sync_meta;
Review Comment:
[P1] Do not mutate a shared cached TabletSchema in place
`sync_meta()` reaches
`mutable_tablet_schema()->set_disable_auto_compaction()`, but
`TabletMeta::init_from_pb()` stores the process-global `TabletSchemaCache`
result keyed only by the serialized schema. Two tablets with identical schemas
therefore share this object. Refreshing tablet A from
`disable_auto_compaction=false` to `true` mutates tablet B as well and leaves
the cache's old false-schema key pointing at true contents, so later false
lookups can also return the poisoned value. This remains wrong even with
perfect locking. Install a schema/cache entry representing the new flag (and
update the handle), or keep this dynamic property outside the shared schema;
add a two-table deduplication test.
--
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]
