yujun777 commented on PR #68390: URL: https://github.com/apache/doris/pull/68390#issuecomment-5882458140
Recording where the four threads this review points at now stand, so the next pass does not have to re-derive it: * **STOP worker lifecycle** (`discussion_r4083719412`) -- resolved. The publication half is fixed, and now on every path the cancel thread can reach: `getIvmCapturedEpochs` hands out a detached copy of the epochs, `after()` hands out a detached copy of the snapshot map, and the write-back filters both to the partitions that are still clean. What the second half of the finding asks for -- publishing and releasing only after the worker quiesces -- is a decision for every job type that shares `AbstractJob`, so it is tracked as its own change rather than this PR. * **Partition-topology alignment** (`discussion_r4084326170`) -- resolved. The stale-snapshot half is fixed: `alignPartitionStates` reads the live partition names itself, under the same MV lock as the map it edits. The window between that read and the `retainAll` is not closed by that lock, because the MV's partition map is mutated under the table's lock; closing it means removing the entry where the partition DDL happens instead of deriving it here, which is a change to the partition lifecycle. * **Cloud omitted partition** (`discussion_r4128592325`) -- answered, deliberately left open. `CloudPartition.hasData()` calls `hasDataCached()` first and answers from the meta service's `get_version` RPC whenever the cached version is not already above `PARTITION_INIT_VERSION` (`cloud/catalog/CloudPartition.java:519-533`, landing in the RPC path at `:175-210`), so a partition whose rows have committed is not answered from a stale cached 1; a stale cache can only make it answer true, which withholds more than it needs to. If there is a path where it answers false with rows present, the line to point at is that comparison -- I have not found one, and the read state's own `visible_version` would be a staler source than what it asks. * **Plain-MV planning cost** (`discussion_r4091824661`) -- kept deliberately. The reason the check captures is observable only in the window the DDL opens (drop a column, add one back under the same name): once the column is back, every later check compares by name and type and passes, so a refresh-time diagnosis reports nothing at all. The cheaper analysis that would keep the window is a change to the shared `MTMVPlanUtil`, not to this one. Since that review, two commits are in: `e952c29714d` (a snapshot read judged by the partitions it selected, before the no-baseline ones are dropped) and `caf968c0c09` (records a partial read holds back carrying the partition they were taken under, and the delta running for them when the recording scope is empty). Their suites and the affected FE unit tests are green locally. -- 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]
