platinumhamburg commented on code in PR #2940:
URL: https://github.com/apache/fluss/pull/2940#discussion_r2994068402
##########
fluss-client/src/main/java/org/apache/fluss/client/write/DynamicPartitionCreator.java:
##########
@@ -121,14 +125,19 @@ private boolean
forceCheckPartitionExist(PhysicalTablePath physicalTablePath) {
return idExist;
}
- private void createPartition(PhysicalTablePath physicalTablePath,
List<String> partitionKeys) {
+ private void createPartition(
+ PhysicalTablePath physicalTablePath,
+ List<String> partitionKeys,
+ AutoPartitionStrategy autoPartitionStrategy) {
String partitionName = physicalTablePath.getPartitionName();
TablePath tablePath = physicalTablePath.getTablePath();
checkArgument(partitionName != null, "Partition name shouldn't be
null.");
ResolvedPartitionSpec resolvedPartitionSpec =
ResolvedPartitionSpec.fromPartitionName(partitionKeys,
partitionName);
+ PartitionSpec partitionSpec = resolvedPartitionSpec.toPartitionSpec();
+ validateAutoPartitionTime(partitionSpec, partitionKeys,
autoPartitionStrategy);
Review Comment:
I think there's a state leak issue in `DynamicPartitionCreator` that could
cause confusing behavior.
In `createPartition()`, `validateAutoPartitionTime` runs **after** the path
has already been added to `inflightPartitionsToCreate` (line 93). If validation
throws `InvalidPartitionException`, the path stays in the set permanently —
`admin.createPartition().whenComplete(...)` never fires, so
`onPartitionCreationFailed` (which does the `remove()`) never runs either.
What happens next: any subsequent write with the same invalid partition
value hits `inflightPartitionsToCreate.add()` → returns `false` → skips
creation entirely → tries to send data to a partition that doesn't exist →
blows up with a different error. Pretty hard to debug from the user's
perspective.
I think the simplest fix is to move the validation before we touch
`inflightPartitionsToCreate` at all — e.g. at the top of
`checkAndCreatePartitionAsync`, right after the null check:
```java
public void checkAndCreatePartitionAsync(
PhysicalTablePath physicalTablePath,
List<String> partitionKeys,
AutoPartitionStrategy autoPartitionStrategy) {
String partitionName = physicalTablePath.getPartitionName();
if (partitionName == null) {
return;
}
// Validate early, before touching any state.
ResolvedPartitionSpec resolvedPartitionSpec =
ResolvedPartitionSpec.fromPartitionName(partitionKeys,
partitionName);
validateAutoPartitionTime(
resolvedPartitionSpec.toPartitionSpec(), partitionKeys,
autoPartitionStrategy);
// ... rest of the existing logic
}
```
This way invalid partitions get rejected immediately without polluting the
inflight set, and as a bonus it removes the redundant `ResolvedPartitionSpec`
construction in `createPartition`.
--
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]
