david-streamlio opened a new pull request, #79:
URL: https://github.com/apache/pulsar-connectors/pull/79
Fixes #78
### Motivation
The DynamoDB source's `awsEndpoint` / `dynamoEndpoint` /
`cloudwatchEndpoint` settings are unusable. Any configuration supplying an
endpoint fails at client construction:
```
java.lang.IllegalStateException: Only one of Region or EndpointConfiguration
may be set.
```
All three builders in `DynamoDBSourceConfig` set an endpoint configuration
and then, in a **separate non-`else` branch**, a region:
```java
if (!this.getAwsEndpoint().isEmpty()) {
builder.setEndpointConfiguration(new
AwsClientBuilder.EndpointConfiguration(...));
}
if (!this.getAwsRegion().isEmpty()) { // not an else
builder.setRegion(this.getAwsRegion());
}
```
AWS SDK v1's `AwsClientBuilder` forbids that pairing — the
`EndpointConfiguration` already carries the signing region. And `awsRegion` is
**mandatory**: `DynamoDBSource.open()` enforces
`checkArgument(isNotBlank(getAwsRegion()), "The aws-region must be set")`. So
whenever an endpoint is configured, both setters run and `build()` throws.
**The endpoint path could never have worked.**
It went unnoticed because production usage against real AWS leaves the
endpoint empty, and the module had no test beyond config parsing (#50). Master
carries a comment that reads like a previous encounter with this, resolved at
the wrong layer:
```java
// Even if the endpoint is set, it seems to require a region to go with it
checkArgument(isNotBlank(dynamodbSourceConfig.getAwsRegion()), "The
aws-region must be set");
```
### Modifications
1. Make region and endpoint **mutually exclusive** (`else if`) in all three
builders.
2. Gate `buildDynamoDBClient` and `buildCloudwatchClient` on their **own**
endpoint fields. They read `dynamoEndpoint` / `cloudwatchEndpoint` while gating
on `awsEndpoint`, so setting `awsEndpoint` alone built an
`EndpointConfiguration` from an empty string.
### Compatibility
I do not believe any working behavior is removed:
- Endpoint + region set → previously threw; now the endpoint wins (carrying
the region as its signing region). Strictly an improvement.
- `awsEndpoint` alone, for the DynamoDB/CloudWatch clients → previously
built an endpoint configuration from an empty string; now falls through to the
region-based default endpoint.
- Both `awsEndpoint` and `dynamoEndpoint` set → unchanged.
- No endpoint set (the common case against real AWS) → unchanged.
### Verifying this change
Existing config tests pass:
```
./gradlew :dynamodb:test
DynamoDBSourceConfigTest > 5 tests PASSED
BUILD SUCCESSFUL
```
The fix is exercised end-to-end by the LocalStack integration test in #77,
which **cannot run without it** — that PR is now test-only and depends on this
one.
--
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]
