Copilot commented on code in PR #698:
URL:
https://github.com/apache/doris-flink-connector/pull/698#discussion_r4024565885
##########
flink-doris-connector/flink-doris-connector-base/src/main/java/org/apache/doris/flink/sink/writer/tvf/S3ClientObjectStore.java:
##########
@@ -35,27 +40,78 @@
public class S3ClientObjectStore implements S3ObjectStore {
private static final String JSON_LINES_CONTENT_TYPE =
"application/x-ndjson";
+ private static final String ROLE_SESSION_NAME = "doris-flink-connector";
Review Comment:
With gzip enabled, objects are uploaded as compressed bytes but the object
metadata is still likely to indicate NDJSON content (via
`JSON_LINES_CONTENT_TYPE`). This mismatch can confuse tooling and consumers
inspecting the bucket. Consider setting `contentEncoding(\"gzip\")` (and/or
adjusting `contentType`) when the `objectKey` ends with `.gz` in `put(...)` so
metadata reflects the actual payload.
##########
flink-doris-connector/flink-doris-connector-base/src/main/java/org/apache/doris/flink/cfg/S3TvfOptions.java:
##########
@@ -159,6 +191,16 @@ public Builder setSecretKey(String secretKey) {
return this;
}
+ public Builder setRoleArn(String roleArn) {
+ this.roleArn = roleArn;
+ return this;
+ }
+
+ public Builder setExternalId(String externalId) {
+ this.externalId = externalId;
+ return this;
+ }
Review Comment:
The builder stores `roleArn` / `externalId` without normalization, but later
validation uses `trim()` only to decide presence. This can allow values like `'
arn:...'` to pass presence checks yet still be used with leading/trailing
whitespace in STS requests / generated SQL. Trim (and ideally null-normalize)
these values in the setters or in `build()` before constructing `S3TvfOptions`.
##########
flink-doris-connector/flink-doris-connector-base/src/main/java/org/apache/doris/flink/table/DorisConfigOptions.java:
##########
@@ -496,16 +526,31 @@ private static void validateTvfLoadProperties(Properties
loadProperties) {
throw new ValidationException(
"TVF write mode requires
'sink.properties.read_json_by_line' to be true.");
}
+ validateTvfCompression(loadProperties);
+ }
+
+ private static void validateTvfCompression(Properties loadProperties) {
+ String compressType = loadProperties.getProperty(COMPRESS_TYPE,
COMPRESS_TYPE_GZ).trim();
+ if (!compressType.isEmpty() &&
!COMPRESS_TYPE_GZ.equalsIgnoreCase(compressType)) {
+ throw new ValidationException(
+ "TVF write mode only supports 'gz' or an empty
'sink.properties.compress_type'.");
Review Comment:
The new compression validation errors are slightly inconsistent across
layers (here it references `sink.properties.compress_type`, while
`DorisExecutionOptions` throws a similar message referencing `compress_type`).
Unifying the phrasing and the exact key name across both validation paths will
make failures easier to diagnose.
##########
flink-doris-connector/flink-doris-connector-flink2/src/test/java/org/apache/doris/flink/table/DorisDynamicTableFactoryTest.java:
##########
@@ -293,8 +293,8 @@ public void testTvfSinkProperties() {
properties.put("sink.s3.region", "us-east-1");
properties.put("sink.s3.bucket", "bucket");
properties.put("sink.s3.prefix", "prefix");
- properties.put("sink.s3.access-key", "ak");
- properties.put("sink.s3.secret-key", "sk");
+ properties.put("sink.s3.role-arn",
"arn:aws:iam::123456789012:role/doris");
+ properties.put("sink.s3.external-id", "external-id");
Review Comment:
This test was switched from static access/secret keys to IAM role options,
but the PR description states both auth modes are supported. To avoid losing
regression coverage for `sink.s3.access-key` / `sink.s3.secret-key` parsing in
the table factory, add a separate assertion path (or an additional test) that
validates the static-credentials configuration still produces the expected
`S3TvfOptions`.
--
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]
