github-actions[bot] commented on code in PR #60953:
URL: https://github.com/apache/doris/pull/60953#discussion_r2928894814
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -2527,6 +2527,28 @@ public int getPartitionTotalReplicasNum(long
partitionId) {
return
partitionInfo.getReplicaAllocation(partitionId).getTotalReplicaNum();
}
+ public Map<Long, List<Long>> getPartitionVersionGapBackends(long
partitionId) {
+ Map<Long, List<Long>> result = new HashMap<>();
+ Partition partition = getPartition(partitionId);
+ if (partition == null) {
Review Comment:
**[Concurrency] Missing read lock.** This method iterates deeply nested
structures (`partition -> index -> tablet -> replica`) without acquiring the
table's `readLock()`. The structurally similar method `getAllTabletIds()` (line
626) acquires `readLock()` for the exact same traversal pattern.
Without the lock, concurrent operations (schema changes adding/dropping
rollup indices, `TabletScheduler` deleting replicas, clone tasks adding
replicas) can modify the underlying `ArrayList`s returned by `getTablets()` and
`getReplicas()` mid-iteration, causing `ConcurrentModificationException`.
Not all callers hold the lock either -- at least 2 of 3 call paths
(`RewriteTableCommand` and `NereidsLoadPlanInfoCollector`) do not hold the
table read lock.
Suggested fix: either acquire `readLock()`/`readUnlock()` within this method
(matching `getAllTabletIds()`), or document that callers must hold it and
ensure they do.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapTableSink.java:
##########
@@ -226,6 +226,10 @@ public void init(TUniqueId loadId, long txnId, long dbId,
long loadChannelTimeou
for (TOlapTablePartition partition :
tOlapTablePartitionParam.getPartitions()) {
partition.setTotalReplicaNum(dstTable.getPartitionTotalReplicasNum(partition.getId()));
partition.setLoadRequiredReplicaNum(dstTable.getLoadRequiredReplicaNum(partition.getId()));
+ Map<Long, List<Long>> gapBackends =
dstTable.getPartitionVersionGapBackends(partition.getId());
Review Comment:
**[Parallel code paths] `FrontendServiceImpl` auto-partition and
replace-partition paths are missing this treatment.**
`FrontendServiceImpl.createPartition()` (~line 4398) and
`FrontendServiceImpl.replacePartition()` (~line 4715) build
`TOlapTablePartition` objects that are sent to BE and processed through the
same `generate_partition_from()` + `_build_tablet_replica_info()` path. Neither
sets `tabletVersionGapBackends` (or `totalReplicaNum`/`loadRequiredReplicaNum`,
though that's a pre-existing issue).
For newly auto-created partitions, replicas are fresh and unlikely to have
version gaps, so the impact is low. But for completeness and correctness, these
paths should populate the same fields. Consider adding the same block after
building `tPartition` in both locations.
##########
be/src/exec/sink/writer/vtablet_writer_v2.cpp:
##########
@@ -1015,13 +1026,15 @@ void VTabletWriterV2::_calc_tablets_to_commit() {
Status VTabletWriterV2::_create_commit_info(std::vector<TTabletCommitInfo>&
tablet_commit_infos,
std::shared_ptr<LoadStreamMap>
load_stream_map) {
- std::unordered_map<int64_t, int> failed_tablets;
+ // Track per-tablet non-gap success count and failure reasons
+ std::unordered_map<int64_t, int> success_tablets_replica;
+ std::unordered_set<int64_t> failed_tablets;
std::unordered_map<int64_t, Status> failed_reason;
load_stream_map->for_each([&](int64_t dst_id, LoadStreamStubs& streams) {
size_t num_success_tablets = 0;
size_t num_failed_tablets = 0;
for (auto [tablet_id, reason] : streams.failed_tablets()) {
- failed_tablets[tablet_id]++;
+ failed_tablets.insert(tablet_id);
Review Comment:
**[Minor] `_create_commit_info` only checks tablets in `failed_tablets`.**
If a tablet has zero failures but all succeeding backends are gap-backends, the
quorum check here is skipped. The FE commit-time check will still catch this
(it's authoritative), so this is not a correctness issue, but it means BE won't
fail-fast in this edge case.
This matches the old code's behavior (which also only checked tablets with
failures), so it's not a regression. Just noting for awareness.
--
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]
