github-actions[bot] commented on code in PR #65282:
URL: https://github.com/apache/doris/pull/65282#discussion_r3535729534
##########
fe/fe-core/src/main/java/org/apache/doris/service/FrontendServiceImpl.java:
##########
@@ -4689,41 +4670,122 @@ public TCreatePartitionResult
createPartition(TCreatePartitionRequest request) t
&& request.isEnableAdaptiveRandomBucket();
boolean loadToSingleTablet = request.isSetLoadToSingleTablet() &&
request.isLoadToSingleTablet();
final boolean hasBeEndpoint = request.isSetBeEndpoint();
- // Lazy: resolved on the first CloudTablet that needs it (skipped on
cache-hit).
- String cachedClusterId = null;
- for (String partitionName : addPartitionClauseMap.keySet()) {
- Partition partition = table.getPartition(partitionName);
- // For thread safety, we preserve the tablet distribution
information of each partition
- // before calling getOrSetAutoPartitionInfo, but not check the
partition first
- List<TTabletLocation> partitionTablets = new ArrayList<>();
- List<TTabletLocation> partitionSlaveTablets = new ArrayList<>();
- TOlapTablePartition tPartition = new TOlapTablePartition();
- tPartition.setId(partition.getId());
- int partColNum = partitionInfo.getPartitionColumns().size();
- try {
- OlapTableSink.setPartitionKeys(tPartition,
partitionInfo.getItem(partition.getId()), partColNum);
- } catch (UserException ex) {
- errorStatus.setErrorMsgs(Lists.newArrayList(ex.getMessage()));
+ class PartitionResultSnapshot {
+ private final Partition partition;
+ private final long partitionId;
+ private final TOlapTablePartition tPartition;
+ private final List<Tablet> tablets;
+ private final int quorum;
+ private final boolean cacheLoadTabletIdx;
+
+ private PartitionResultSnapshot(Partition partition, long
partitionId,
+ TOlapTablePartition tPartition, List<Tablet> tablets, int
quorum, boolean cacheLoadTabletIdx) {
+ this.partition = partition;
+ this.partitionId = partitionId;
+ this.tPartition = tPartition;
+ this.tablets = tablets;
+ this.quorum = quorum;
+ this.cacheLoadTabletIdx = cacheLoadTabletIdx;
+ }
+ }
+
+ List<PartitionResultSnapshot> partitionSnapshots = new ArrayList<>();
+
+ olapTable.readLock();
+ try {
Review Comment:
Can we apply this same snapshot pattern to `replacePartition` before
returning? The BE auto-detect overwrite path calls
`FrontendService.replacePartition` with the same routing fields (`be_endpoint`,
`load_to_single_tablet`, `enable_adaptive_random_bucket`, `query_id`) from
`VRowDistribution::_replace_overwriting_partition`. In `replacePartition`,
after `taskLock` is released, the response builder still does
`olapTable.getPartition(partitionId)` and immediately dereferences
`partition.getId()` / `partitionInfo.getItem(...)` without a table read lock or
null check. `InsertOverwriteManager.taskGroupFail` can acquire the task lock
after this point and roll back the temp partitions via
`InsertOverwriteUtil.dropPartitions`, so a failed or cancelled auto-detect
overwrite can still race this response builder into an FE NPE instead of a
normal error status. Please snapshot the replace result metadata under the
table read lock, or otherwise keep the task/table protection until the metadata
needed for the response has been captured, and return a retry/failure status
if a result partition or item has already disappeared.
--
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]
