Copilot commented on code in PR #54638:
URL: https://github.com/apache/doris/pull/54638#discussion_r2277369237
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/InternalCatalog.java:
##########
@@ -1646,7 +1646,69 @@ public PartitionPersistInfo addPartition(Database db,
String tableName, AddParti
Preconditions.checkNotNull(olapTable);
Preconditions.checkNotNull(indexIdToMeta);
+ // Guard to serialize concurrent creation of the same partition within
the same table.
+ // We acquire or wait on a per-partition future to ensure only one
creator proceeds to heavy work.
+ CompletableFuture<Void> ownerFuture = null;
+ while (true) {
+ Pair<CompletableFuture<Void>, Boolean> acquire =
olapTable.acquirePartitionCreationFuture(partitionName);
+ if (acquire.second) {
+ // we have the ownership. do the creation work and share the
result by completePartitionCreationFuture.
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("acquired owner for table[{}] partition[{}],
tableId={}", tableName, partitionName,
+ olapTable.getId());
+ }
+ ownerFuture = acquire.first;
+ break;
+ } else {
+ // Wait for ongoing creation to finish, then double-check
existence.
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("waiting for ongoing creation of table[{}]
partition[{}]", tableName, partitionName);
+ }
+ try {
+ acquire.first.join();
+ } catch (RuntimeException ex) {
+ // ignore the exception. we only care about it's created
or not. will check below.
+ if (LOG.isDebugEnabled()) {
Review Comment:
The log level is DEBUG in the if condition but WARN in the actual logging
call. This inconsistency could lead to unexpected log output when debug logging
is enabled.
```suggestion
if (LOG.isWarnEnabled()) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -225,6 +226,10 @@ public enum OlapTableState {
private volatile Statistics statistics = new Statistics();
+ // Transient map to coordinate concurrent partition creation tasks per
partition name.
+ // Ensures only one creation task runs for a given partition at a time.
+ private ConcurrentHashMap<String, CompletableFuture<Void>>
partitionCreationFutures = new ConcurrentHashMap<>();
Review Comment:
The field should be marked as transient since it's initialized in
gsonPostProcess() and represents runtime state that shouldn't be serialized.
```suggestion
private transient ConcurrentHashMap<String, CompletableFuture<Void>>
partitionCreationFutures = new ConcurrentHashMap<>();
```
--
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]
