Copilot commented on code in PR #15122: URL: https://github.com/apache/iceberg/pull/15122#discussion_r2777583399
########## docs/docs/aws.md: ########## @@ -659,6 +659,25 @@ spark-sql --conf spark.sql.catalog.my_catalog=org.apache.iceberg.spark.SparkCata For more details on using S3 Dual-stack, please refer [Using dual-stack endpoints from the AWS CLI and the AWS SDKs](https://docs.aws.amazon.com/AmazonS3/latest/userguide/dual-stack-endpoints.html#dual-stack-endpoints-cli) +### S3 Metrics Publisher + +`S3FileIO` supports configuring a custom [MetricPublisher](https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/metrics/MetricPublisher.html) to capture metrics emitted by the AWS SDK for S3 operations. Note that this is not supported when using the [S3 CRT client](https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/crt-based-s3-client.html) (`s3.crt.enabled=true`). + +To enable, set `s3.metrics-publisher-impl` to a class implementing `MetricPublisher`. The class must provide either a static `create(Map<String, String>)` factory method or a no-arg constructor. + +| Property | Default | Description | +| -------------------------- | ------- | --------------------------------------------------------------------------- | +| s3.metrics-publisher-impl | null | Fully qualified class name of a `MetricPublisher` implementation | + +For example, to use a custom metrics publisher with Spark 3.5: Review Comment: The example class name `com.example.MyMetricsConfig` is a bit misleading given the property expects a `MetricPublisher` implementation. Consider renaming it to something like `com.example.MyMetricPublisher` (or similar) to make the required type obvious to readers. ########## aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java: ########## @@ -1134,6 +1146,51 @@ public <T extends S3ClientBuilder> void applyS3AccessGrantsConfigurations(T buil } } + public <T extends S3BaseClientBuilder<T, ?>> void applyMetricsPublisherConfiguration(T builder) { + if (metricsPublisherImpl != null) { + MetricPublisher metricPublisher = loadMetricPublisher(metricsPublisherImpl, allProperties); + ClientOverrideConfiguration.Builder configBuilder = + builder.overrideConfiguration() != null + ? builder.overrideConfiguration().toBuilder() + : ClientOverrideConfiguration.builder(); + builder.overrideConfiguration(configBuilder.addMetricPublisher(metricPublisher).build()); + } + } Review Comment: Docs state this is not supported when using the S3 CRT client (`s3.crt.enabled=true`), but the code applies the MetricPublisher unconditionally. Add an explicit guard here (e.g., throw a validation/illegal-argument exception with a clear message, or skip applying) when `isS3CRTEnabled` is true and `metricsPublisherImpl` is set, so runtime behavior matches the documented constraint. ########## aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java: ########## @@ -1134,6 +1146,51 @@ public <T extends S3ClientBuilder> void applyS3AccessGrantsConfigurations(T buil } } + public <T extends S3BaseClientBuilder<T, ?>> void applyMetricsPublisherConfiguration(T builder) { + if (metricsPublisherImpl != null) { + MetricPublisher metricPublisher = loadMetricPublisher(metricsPublisherImpl, allProperties); + ClientOverrideConfiguration.Builder configBuilder = + builder.overrideConfiguration() != null + ? builder.overrideConfiguration().toBuilder() + : ClientOverrideConfiguration.builder(); + builder.overrideConfiguration(configBuilder.addMetricPublisher(metricPublisher).build()); + } + } + + /** + * Load a MetricPublisher implementation. Tries static create(Map) factory method first, falls + * back to no-arg constructor. + */ + private MetricPublisher loadMetricPublisher(String impl, Map<String, String> properties) { + // Try static create(Map) factory method first + try { + return DynMethods.builder("create") + .hiddenImpl(impl, Map.class) + .buildStaticChecked() + .invoke(properties); + } catch (NoSuchMethodException e) { Review Comment: If the `create(Map)` method exists but throws (or returns an incompatible type), this path will likely surface a less actionable exception than the later `IllegalArgumentException` you build for constructors. Consider catching broader failures from the `invoke(...)` path and rethrowing an `IllegalArgumentException` that clearly states instantiation via `create(Map)` failed for the given class, including the original cause. -- 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]
