deepakpanda93 commented on code in PR #19591:
URL: https://github.com/apache/hudi/pull/19591#discussion_r3764927326
##########
website/docs/concurrency_control.md:
##########
@@ -188,6 +188,32 @@ com.amazonaws:aws-java-sdk-dynamodb
com.amazonaws:aws-java-sdk-core
```
+### DynamoDB-Based Lock Provider with Implicit Partition Key
+
+```properties
+hoodie.write.lock.provider=org.apache.hudi.aws.transaction.lock.DynamoDBBasedImplicitPartitionKeyLockProvider
+```
+
+This variant behaves like the DynamoDB-based lock provider above, except in
how it determines the DynamoDB partition
+key. Rather than reading `hoodie.write.lock.dynamodb.partition_key`, it
derives the key from the table's base path: the
+64-bit xxHash of that path, with `s3a://` normalized to `s3://` so that
writers reaching the same table through either
+scheme take the same lock.
+
+Prefer it when many tables share one lock table. The standard provider requires
Review Comment:
Good catch on both halves — the sentence was ambiguous, and tracing it
showed neither "requires" nor "falls back" is the whole story. Fixed in
ef92ee8ea74a.
**What actually happens.** `DynamoDbBasedLockConfig.from(properties)` copies
the caller's props and then calls `setDefaults(...)`:
```java
DynamoDbBasedLockConfig config = new DynamoDbBasedLockConfig();
config.getProps().putAll(properties);
config.setDefaults(DynamoDbBasedLockConfig.class.getName());
```
and `HoodieConfig.setDefaultValue` **materializes** an inferred value into
the props when the property has an infer function:
```java
if (configProperty.hasInferFunction()) {
inferValue = configProperty.getInferFunction().get().apply(this);
}
if (inferValue.isPresent() || configProperty.hasDefaultValue()) {
props.setProperty(configProperty.key(), inferValue.isPresent() ?
inferValue.get().toString() : ...);
}
```
Only afterwards does `DynamoDBBasedLockProvider.getDynamoDBPartitionKey` run
`checkArgument(config.contains(DYNAMODB_LOCK_PARTITION_KEY), ...)`, and
`HoodieConfig.contains` consults only the props map (plus alternatives) — not
the infer function. So:
| props contain | outcome |
|---|---|
| `hoodie.write.lock.dynamodb.partition_key` | used as given |
| only `hoodie.table.name` | inference writes partition_key into props,
`contains` passes, key = table name |
| neither | `contains` fails → `Config key is not found:
hoodie.write.lock.dynamodb.partition_key` |
So it does fall back on every normal write, *and* the `checkArgument` is
still reachable when the table name is absent too. Your suggested "(rather than
failing)" would have been very nearly right but would have closed off that
corner case, so I went with wording that asserts neither extreme:
> The standard provider takes its partition key from
`hoodie.write.lock.dynamodb.partition_key`, which you rarely set: when it is
absent, Hudi fills it in from the table name. Two tables that happen to share a
name, in different databases or under different paths, therefore resolve to the
same lock and serialize writers that never touch the same data.
That keeps the collision motivation — which is the point of the paragraph —
without making a claim about failure behaviour in either direction.
**On the tension with the section above**: you are right, and the
pre-existing wording was the looser of the two. `DYNAMODB_LOCK_PARTITION_KEY`
is declared `.noDefaultValue()` with `.withInferFunction(cfg -> ...
HoodieTableConfig.NAME ...)`, so "default: table name" describes the effect but
not the mechanism. Changed that parenthetical to "(inferred from the table name
when unset)" in the three copies that carry it — `next`, 1.2.0 and 1.1.1. The
1.0.x copies word the same point differently and needed no change, so nothing
was touched there.
Flagging that as a deliberate touch of pre-existing text: it is one
parenthetical, inside the section this PR extends, and leaving the two
sentences disagreeing about the same config would have been worse.
Build clean with the warning set byte-identical to a baseline of the same
base commit; `default: table name` now appears in zero files.
--
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]
