yadavay-amzn commented on code in PR #16235:
URL: https://github.com/apache/iceberg/pull/16235#discussion_r3483245314
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java:
##########
@@ -1197,4 +1209,63 @@ private <T> T loadSdkPluginConfigurations(String impl,
Map<String, String> prope
public Map<String, String> properties() {
return allProperties;
}
+
+ public String metricsPublisherImpl() {
+ return metricsPublisherImpl;
+ }
+
+ /**
+ * Configure a custom {@link MetricPublisher} for an S3 client.
+ *
+ * <p>Sample usage:
+ *
+ * <pre>
+ *
S3Client.builder().applyMutation(s3FileIOProperties::applyMetricsPublisherConfiguration)
+ * </pre>
+ */
+ public <T extends S3BaseClientBuilder<T, ?>> void
applyMetricsPublisherConfiguration(T builder) {
+ if (metricsPublisherImpl != null) {
+ ClientOverrideConfiguration.Builder configBuilder =
+ null != builder.overrideConfiguration()
+ ? builder.overrideConfiguration().toBuilder()
+ : ClientOverrideConfiguration.builder();
+ builder.overrideConfiguration(
+ configBuilder.addMetricPublisher(loadMetricPublisher()).build());
+ }
+ }
+
+ private MetricPublisher loadMetricPublisher() {
+ // Phase 1: look up the factory. A NoSuchMethodException here means the
class does not
+ // declare `create(Map)` — fall back to the no-arg constructor path. Any
OTHER exception
+ // from a factory that DOES exist (e.g. the factory itself throws) should
surface via the
+ // wrapping IllegalArgumentException in phase 2 so users can distinguish
"wrong signature"
+ // from "misconfigured factory".
+ DynMethods.StaticMethod factory = null;
+ try {
+ factory =
+ DynMethods.builder("create")
+ .hiddenImpl(metricsPublisherImpl, Map.class)
+ .buildStaticChecked();
Review Comment:
You're right - verified against the SDK (2.42.41): none of
`CloudWatchMetricPublisher`, `LoggingMetricPublisher`, or
`EmfMetricLoggingPublisher` have `create(Map)` or a public no-arg constructor
(CloudWatch/Logging expose a static `create()`; EMF is builder-only), so the
original mechanism instantiated none of them.
Reworked it to load a small user-provided factory instead: a new
`S3MetricPublisherProvider` interface (no-arg constructor +
`initialize(Map<String,String>)` + `metricPublisher()`), configured via
`s3.metrics-publisher-impl` and loaded with `DynConstructors` - mirroring how
`AwsClientFactory` / `FileIO` / `MetricsReporter` are loaded. The user's
provider wraps whichever publisher they want (CloudWatch/Logging via `create()`
or the builder, EMF via its builder) and passes configuration, so it works with
all three including the builder-only one. aws.md now has a CloudWatch example.
Does this approach look okay to you?
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java:
##########
@@ -1197,4 +1209,63 @@ private <T> T loadSdkPluginConfigurations(String impl,
Map<String, String> prope
public Map<String, String> properties() {
return allProperties;
}
+
+ public String metricsPublisherImpl() {
+ return metricsPublisherImpl;
+ }
+
+ /**
+ * Configure a custom {@link MetricPublisher} for an S3 client.
+ *
+ * <p>Sample usage:
+ *
+ * <pre>
+ *
S3Client.builder().applyMutation(s3FileIOProperties::applyMetricsPublisherConfiguration)
+ * </pre>
+ */
+ public <T extends S3BaseClientBuilder<T, ?>> void
applyMetricsPublisherConfiguration(T builder) {
+ if (metricsPublisherImpl != null) {
+ ClientOverrideConfiguration.Builder configBuilder =
+ null != builder.overrideConfiguration()
+ ? builder.overrideConfiguration().toBuilder()
+ : ClientOverrideConfiguration.builder();
+ builder.overrideConfiguration(
+ configBuilder.addMetricPublisher(loadMetricPublisher()).build());
+ }
+ }
+
+ private MetricPublisher loadMetricPublisher() {
+ // Phase 1: look up the factory. A NoSuchMethodException here means the
class does not
+ // declare `create(Map)` — fall back to the no-arg constructor path. Any
OTHER exception
+ // from a factory that DOES exist (e.g. the factory itself throws) should
surface via the
+ // wrapping IllegalArgumentException in phase 2 so users can distinguish
"wrong signature"
+ // from "misconfigured factory".
+ DynMethods.StaticMethod factory = null;
+ try {
+ factory =
+ DynMethods.builder("create")
+ .hiddenImpl(metricsPublisherImpl, Map.class)
+ .buildStaticChecked();
+ } catch (NoSuchMethodException e) {
+ // Expected when the implementation doesn't provide a create(Map)
factory — fall through.
+ }
+
+ // Phase 2: invoke whichever path we found. Exceptions here are real
failures and are
+ // surfaced with the precise path that failed so the user can diagnose.
+ try {
+ if (factory != null) {
+ return (MetricPublisher) factory.invoke(allProperties);
+ }
+ return Class.forName(metricsPublisherImpl)
+ .asSubclass(MetricPublisher.class)
+ .getDeclaredConstructor()
+ .newInstance();
+ } catch (Exception e) {
Review Comment:
Done - the negative tests now assert the specific exception/message (missing
no-arg constructor; not an `S3MetricPublisherProvider`).
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestS3FileIOProperties.java:
##########
@@ -587,4 +589,154 @@ public void testChunkedEncodingDisabled() {
.as("chunked encoding should be disabled when explicitly set to false")
.isFalse();
}
+
+ @Test
+ public void testApplyMetricsPublisherConfigurationWithFactoryMethod() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(
+ S3FileIOProperties.METRICS_PUBLISHER_IMPL,
FactoryMetricPublisher.class.getName());
+ S3FileIOProperties s3FileIOProperties = new S3FileIOProperties(properties);
+
+ S3ClientBuilder builder = S3Client.builder();
+ s3FileIOProperties.applyMetricsPublisherConfiguration(builder);
+
+ assertThat(builder.overrideConfiguration()).isNotNull();
+ assertThat(builder.overrideConfiguration().metricPublishers()).hasSize(1);
+ assertThat(builder.overrideConfiguration().metricPublishers().get(0))
+ .isInstanceOf(FactoryMetricPublisher.class);
+ }
+
+ @Test
+ public void testApplyMetricsPublisherConfigurationWithNoArgConstructor() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(S3FileIOProperties.METRICS_PUBLISHER_IMPL,
NoArgMetricPublisher.class.getName());
+ S3FileIOProperties s3FileIOProperties = new S3FileIOProperties(properties);
+
+ S3ClientBuilder builder = S3Client.builder();
+ s3FileIOProperties.applyMetricsPublisherConfiguration(builder);
+
+ assertThat(builder.overrideConfiguration()).isNotNull();
+ assertThat(builder.overrideConfiguration().metricPublishers()).hasSize(1);
+ assertThat(builder.overrideConfiguration().metricPublishers().get(0))
+ .isInstanceOf(NoArgMetricPublisher.class);
+ }
Review Comment:
The rework changes this - instead of enumerating SDK publisher classes,
users now supply a single provider class, so the positive test loads one
provider; the two negative cases are separate tests since their expected
messages differ (missing no-arg constructor vs. not an
`S3MetricPublisherProvider`). Happy to parameterize if you would still prefer.
##########
docs/docs/aws.md:
##########
@@ -659,6 +659,24 @@ 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 Custom MetricPublisher
+
+A custom `MetricPublisher` implementation can be plugged into the S3 client by
setting the `s3.metrics-publisher-impl` catalog property to the fully qualified
class name of a class that implements
`software.amazon.awssdk.metrics.MetricPublisher`.
Review Comment:
Done - added the SDK v2 `MetricPublisher` javadoc link in aws.md.
##########
aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java:
##########
@@ -1197,4 +1209,63 @@ private <T> T loadSdkPluginConfigurations(String impl,
Map<String, String> prope
public Map<String, String> properties() {
return allProperties;
}
+
+ public String metricsPublisherImpl() {
+ return metricsPublisherImpl;
+ }
+
+ /**
+ * Configure a custom {@link MetricPublisher} for an S3 client.
+ *
+ * <p>Sample usage:
+ *
+ * <pre>
+ *
S3Client.builder().applyMutation(s3FileIOProperties::applyMetricsPublisherConfiguration)
+ * </pre>
+ */
+ public <T extends S3BaseClientBuilder<T, ?>> void
applyMetricsPublisherConfiguration(T builder) {
Review Comment:
Done - renamed to `applyMetricsPublisherConfigurations` (plural).
--
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]
