david-streamlio commented on PR #77: URL: https://github.com/apache/pulsar-connectors/pull/77#issuecomment-4930026472
Heads-up for reviewers: **this PR changes production code**, not just tests, and that is deliberate — the integration test cannot reach LocalStack without it. The defect is filed separately as #78. Independently verified: - `aws-java-sdk-core` contains the literal message `Only one of Region or EndpointConfiguration may be set.`, so setting both is rejected at `build()`. - `DynamoDBSource.open()` mandates `awsRegion` (`checkArgument(isNotBlank(...), "The aws-region must be set")`), so the region branch always runs. Whenever an endpoint is configured, both setters fire and construction throws. The endpoint path could never have worked. - Master even carries a comment acknowledging the friction — `// Even if the endpoint is set, it seems to require a region to go with it` — which reads like this was hit before and worked around at the wrong layer. The second change (gating `buildDynamoDBClient` / `buildCloudwatchClient` on `dynamoEndpoint` / `cloudwatchEndpoint` rather than `awsEndpoint`) is a behavior change worth a close look, though I don't believe it removes any working behavior: previously, setting `awsEndpoint` alone built an `EndpointConfiguration` from an **empty** `dynamoEndpoint`, which was already broken. After the change that case falls through to the region-based default endpoint. Users setting both fields are unaffected. If maintainers would rather keep the fix and the test separate, I'm happy to split #78's fix into its own PR and rebase this one on top. -- 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]
