github-actions[bot] commented on code in PR #66756:
URL: https://github.com/apache/doris/pull/66756#discussion_r3789509073
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -4530,13 +4530,11 @@ public TStatus
updateStatsCache(TUpdateFollowerStatsCacheRequest request) throws
StatisticsCacheKey k = GsonUtils.GSON.fromJson(request.key,
StatisticsCacheKey.class);
ColStatsData data = GsonUtils.GSON.fromJson(request.colStatsData,
ColStatsData.class);
ColumnStatistic c = data.toColumnStatistic();
- if (c == ColumnStatistic.UNKNOWN) {
-
Env.getCurrentEnv().getStatisticsCache().invalidateColumnStatsCache(k.catalogId,
k.dbId, k.tableId,
- k.idxId, k.colName);
- } else {
- Env.getCurrentEnv().getStatisticsCache().updateColStatsCache(
- k.catalogId, k.dbId, k.tableId, k.idxId, k.colName, c);
- }
+ // Publish a blocking UNKNOWN instead of invalidating, mirroring the
leader-side
+ // syncColStats: invalidation would let a concurrent async loader
re-install the
+ // previous stale row before the leader's buffered insert becomes
visible here.
+ Env.getCurrentEnv().getStatisticsCache().updateColStatsCache(
Review Comment:
[P2] Keep transient follower conversion misses retryable —
`toColumnStatistic()` returns the same UNKNOWN both for the invalid-stat shape
and when this follower has not replayed the referenced column/index yet. The
stats RPC has no journal barrier, so a valid update can arrive first; this
unconditional put then leaves UNKNOWN cached after metadata catches up, with no
replay invalidation until the 48-hour refresh. This is distinct from the
existing invalid-row race. Please publish a blocking UNKNOWN only when
`!data.isValid()`; keep other conversion failures invalidated/retryable, and
test RPC-before-schema-replay recovery.
##########
fe/fe-core/src/main/java/org/apache/doris/statistics/BaseAnalysisTask.java:
##########
@@ -667,9 +667,12 @@ protected void runQuery(String sql) {
if (MetricRepo.isInit) {
MetricRepo.COUNTER_STATISTICS_INVALID_STATS.increase(1L);
}
- String message = String.format("ColStatsData is invalid, skip
analyzing. %s", colStatsData.toSQL(true));
- LOG.warn(message);
- throw new RuntimeException(message);
+ // Don't throw: keep writing the row into the statistics table
so that the
Review Comment:
[P1] Fence persisted invalid rows from old FE readers — The new contract
depends on both new `isValid()` guards, but this branch now sends and persists
the contradictory row while a base-version FE still converts it into ordinary
statistics. During rolling FE upgrade, an upgraded follower can become master
after failover while an old follower remains; that old FE then caches the RPC
payload and later reloads the same bogus row from the shared table. This is
distinct from the existing pre-flush cache-race thread on this line. Please
gate persistence until all FEs support the reader contract or use a
backward-compatible representation/version fence, and cover the immediately
previous reader behavior.
##########
fe/fe-core/src/test/java/org/apache/doris/statistics/BaseAnalysisTaskTest.java:
##########
@@ -19,14 +19,20 @@
import org.apache.doris.analysis.TableSample;
import org.apache.doris.catalog.Column;
+import org.apache.doris.catalog.Env;
import org.apache.doris.catalog.PrimitiveType;
+import org.apache.doris.qe.ConnectContext;
import org.apache.doris.qe.StmtExecutor;
+import org.apache.doris.thrift.TUniqueId;
import com.google.common.collect.Lists;
+import mockit.Expectations;
Review Comment:
[P1] Use `fe-core`'s declared mocking framework — These are the only real
`mockit.*` imports in `fe-core`, but this module declares only
`mockito-core`/`mockito-inline`; neither its POM nor the FE parent provides
`org.jmockit:jmockit`, and Surefire has no JMockit javaagent. The new
deterministic test therefore cannot compile/run in the FE test target. This is
distinct from the existing request for deterministic coverage: the supplied
replacement itself is unbuildable. Please express it with the already-declared
Mockito stack (the removed test already used `MockedConstruction`), or
explicitly add and configure JMockit for this module.
--
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]
