You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rmannibucau (via GitHub)" <gi...@apache.org> on 2026/02/12 11:47:45 UTC
[PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
rmannibucau opened a new pull request, #15304:
URL: https://github.com/apache/iceberg/pull/15304
(no comment)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-3895503259
@rcjverhoef do you want to add the properties filter + `create()` - noarg - support in your PR to align on credential provider pattern? Or do we align credential provider one on your PR which is passing all properties? :thinking:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#discussion_r3015894813
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
+ @Test
+ void metricsPublisherCreateSync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ "client.credentials-provider", TestCredentialProvider.class.getName(),
+ "client.metrics-publisher", NoArgPublisher.class.getName(),
+ "client.region", "us-east-1"));
+ try (var s3 = factory.s3()) {
+ assertThat(NoArgPublisher.INSTANCE.get()).isNotNull();
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateMapSync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ "client.credentials-provider",
+ TestCredentialProvider.class.getName(),
+ "client.metrics-publisher",
+ MapArgPublisher.class.getName(),
+ "client.metrics-publisher.test",
+ "ok",
+ "client.region",
+ "us-east-1"));
+ try (var s3 = factory.s3()) {
+ assertThat(MapArgPublisher.INSTANCE.get().args.get("test")).isEqualTo("ok");
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateASync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ // CRT impl is still not equivalent to the default java one
+ "s3.crt.enabled",
+ "false",
+ "client.credentials-provider",
+ TestCredentialProvider.class.getName(),
+ "client.metrics-publisher",
+ NoArgPublisher.class.getName(),
+ "client.region",
+ "us-east-1"));
+ try (var s3 = factory.s3Async()) {
+ assertThat(NoArgPublisher.INSTANCE.get()).isNotNull();
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateMapASync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ // CRT impl is still not equivalent to the default java one
+ "s3.crt.enabled", "false",
Review Comment:
just noting that I'm not a fan of the CRT as despite claims, it's not better than a well engineered aws client app and a source of pain for all
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
Review Comment:
I think you need some tests to validate the failure paths of the code
* class not found
* failure to instantiate
* not assignable
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#discussion_r3021148661
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -140,6 +201,36 @@ public void close() {
}
}
+ public static class NoCreatePublisher implements MetricPublisher {
+ @Override
+ public void publish(MetricCollection metricCollection) {
+ // no-op
+ }
+
+ @Override
+ public void close() {
+ // no-op
+ }
+ }
+
+ public static class OopsPublisher implements MetricPublisher {
Review Comment:
I'd suggest a more boring name like "PublisherThatFailsToCreate"
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
Review Comment:
common in my code, so it's always something I look for in tests. All to common in production too š
Note that I'm not personally a strong fan of testing for text, but iceberg tooling expects it. My PoV: if you do test for text, have the text in a constant in production code and look for that. But again, not common in this repo, so I'm not going to suggest it in this PR.
What is important _and I've been burned by it_ is don't look for error text from AWS services.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5459142667
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think thatās incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#discussion_r2799743896
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
+ @Test
+ void metricsPublisherCreateSync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ "client.credentials-provider", TestCredentialProvider.class.getName(),
+ "client.metrics-publisher", NoArgPublisher.class.getName(),
+ "client.region", "us-east-1"));
+ try (var s3 = factory.s3()) {
+ assertThat(NoArgPublisher.INSTANCE.get()).isNotNull();
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateMapSync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ "client.credentials-provider",
+ TestCredentialProvider.class.getName(),
+ "client.metrics-publisher",
+ MapArgPublisher.class.getName(),
+ "client.metrics-publisher.test",
+ "ok",
+ "client.region",
+ "us-east-1"));
+ try (var s3 = factory.s3()) {
+ assertThat(MapArgPublisher.INSTANCE.get().args.get("test")).isEqualTo("ok");
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateASync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ // CRT impl is still not equivalent to the default java one
+ "s3.crt.enabled",
+ "false",
+ "client.credentials-provider",
+ TestCredentialProvider.class.getName(),
+ "client.metrics-publisher",
+ NoArgPublisher.class.getName(),
+ "client.region",
+ "us-east-1"));
+ try (var s3 = factory.s3Async()) {
+ assertThat(NoArgPublisher.INSTANCE.get()).isNotNull();
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateMapASync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ // CRT impl is still not equivalent to the default java one
+ "s3.crt.enabled", "false",
Review Comment:
not sure I got this one, issue is CRT doesn't support metrics publisher so I disable it explicitly in tests
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#discussion_r3016246954
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
Review Comment:
Added but doesnt seem a common practise in this module at least (guess cause it reuses shared code?) :thinking:
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5952477631
up?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-3941841477
how do we move forward then? anything to rework in the PR?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-4365048630
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think thatās incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5055246310
any hope it gets integrated as a feature - even if not this PR? right now metrics are very partial to tune properly AWS setup
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5053039849
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think thatās incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-4160281765
Hi everyone, aything missing there?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-4130716955
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think thatās incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5120285902
up? almost half a year for such a critical feature, did I miss another way to get it?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-4667086919
up? these metrics are still key for observability and performance tuning
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-3891844125
https://github.com/apache/iceberg/pull/15122 works for me but doesnt expose any configuration - not a blocker here since I end up storing in a static copyonwritelist the instances to wire them in a spark sink but I assume we want to align the model on credential provider one?
No issue to close this PR if the other one is preferred while it hits a release at some point :)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-4665449521
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think thatās incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#discussion_r3021495685
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
Review Comment:
hehe, or translated text ;)
anyway, renamed the oops publisher/exception so guess we are better now
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5615578696
anything holding that?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-3892384603
just suggesting you work together to produce something which you both find works, as that's how best to get something in which suits more people
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "steveloughran (via GitHub)" <gi...@apache.org>.
steveloughran commented on code in PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#discussion_r2799197893
##########
aws/src/test/java/org/apache/iceberg/aws/s3/TestDefaultS3FileIOAwsClientFactory.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.aws.s3;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junitpioneer.jupiter.SetSystemProperty;
+import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentials;
+import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
+import software.amazon.awssdk.metrics.MetricCollection;
+import software.amazon.awssdk.metrics.MetricPublisher;
+
+// disable loading (unexpected) profiles
+@SetSystemProperty(key = "aws.sharedCredentialsFile", value = "target/missing/ignore")
+@SetSystemProperty(key = "aws.configFile", value = "target/missing/ignore")
+class TestDefaultS3FileIOAwsClientFactory {
+ @Test
+ void metricsPublisherCreateSync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ "client.credentials-provider", TestCredentialProvider.class.getName(),
+ "client.metrics-publisher", NoArgPublisher.class.getName(),
+ "client.region", "us-east-1"));
+ try (var s3 = factory.s3()) {
+ assertThat(NoArgPublisher.INSTANCE.get()).isNotNull();
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateMapSync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ "client.credentials-provider",
+ TestCredentialProvider.class.getName(),
+ "client.metrics-publisher",
+ MapArgPublisher.class.getName(),
+ "client.metrics-publisher.test",
+ "ok",
+ "client.region",
+ "us-east-1"));
+ try (var s3 = factory.s3()) {
+ assertThat(MapArgPublisher.INSTANCE.get().args.get("test")).isEqualTo("ok");
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateASync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ // CRT impl is still not equivalent to the default java one
+ "s3.crt.enabled",
+ "false",
+ "client.credentials-provider",
+ TestCredentialProvider.class.getName(),
+ "client.metrics-publisher",
+ NoArgPublisher.class.getName(),
+ "client.region",
+ "us-east-1"));
+ try (var s3 = factory.s3Async()) {
+ assertThat(NoArgPublisher.INSTANCE.get()).isNotNull();
+ } finally {
+ NoArgPublisher.INSTANCE.remove();
+ }
+ }
+
+ @Test
+ void metricsPublisherCreateMapASync() {
+ final var factory = new DefaultS3FileIOAwsClientFactory();
+ factory.initialize(
+ Map.of(
+ // CRT impl is still not equivalent to the default java one
+ "s3.crt.enabled", "false",
Review Comment:
just a source of a different stack traces...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rcjverhoef (via GitHub)" <gi...@apache.org>.
rcjverhoef commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-3944325212
> how do we move forward then? anything to rework in the PR?
Not from my side. We can go with this one. Main challenge for me so far was getting an Iceberg maintainer to review and approve. Happy to close out mine and go with this
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rcjverhoef (via GitHub)" <gi...@apache.org>.
rcjverhoef commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-3940631229
> @rcjverhoef do you want to add the properties filter + `create()` - noarg - support in your PR to align on credential provider pattern? Or do we align credential provider one on your PR which is passing all properties? š¤
@rmannibucau your PR takes a more generic approach, placing the config in AwsClientProperties aligned with the credential provider pattern. Mine is scoped to S3FileIO specifically, configuring the metrics publisher per S3 client instance.
What I reversed engineered so far, is that every S3FileIO gets its own AWS client. This is particular useful for vended credentials and remote signing, but there is probably more. I am hooking the metrics publisher in at that level.
The more I think about it, I am wondering if your approach makes more sense. I don't really know under which circumstances you'd want that have different metrics publishers for each client.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-4366065198
up?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Enable to configure metrics-publisher in AWS client factory [iceberg]
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on PR #15304:
URL: https://github.com/apache/iceberg/pull/15304#issuecomment-5461521419
up? what does block to get AWS monitoring in?
@rdblue - sorry to ping directly but makes 6 months without any feedback from core committers - any hope to get a review/merge/follow up on the feature?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org