You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/03/02 20:45:51 UTC
[GitHub] [iceberg] kunal0829 opened a new pull request #4254: Dynamic MetricsContext Initialization
kunal0829 opened a new pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254
With MetricsContext being recently added, it is important to be able to dynamically initialize what implementation of MetricsContext the user prefers. This PR adds that option with the `io.metrics-impl` property. Moved `IO_METRICS_IMPL` and `DEFAULT_METRICS_IMPL` to `CatalogProperties` to keep consistency with the repo.
--
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
[GitHub] [iceberg] kunal0829 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
kunal0829 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819201759
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
For both statsd and prometheus, it is called namespace. Collectd takes a combination of host and plugin to create a "namespace".
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821223673
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_NAMESPACE = "io.metrics.namespace";
Review comment:
I find this name confusing because I don't think of a metrics namespace. This is specific to Hadoop, right?
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r820932605
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
@danielcweeks the code is updated, any more comments about 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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821157522
##########
File path: api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java
##########
@@ -29,6 +29,21 @@
* like thread safety and serialization.
*/
public interface MetricsContext extends Serializable {
+ MetricsContext NULL_METRICS = new MetricsContext() {
Review comment:
Then we should move it outside of the interface? No need to have both NULL_METRICS and `nullMetrics()`.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819797231
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
sounds like using a term `io.metrics.namespace` seems like a more generic solution? @danielcweeks Hadoop can map namespace to scheme
--
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
[GitHub] [iceberg] rdblue commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062165212
> (regarding metrics namespace, it was discussed in https://github.com/apache/iceberg/pull/4254#discussion_r819178970, it seems to be the terminology that most metrics system use, let me know what you think)
I think the problem is that we're standardizing something that doesn't need to be standard. This is specific to Hadoop metrics collection. I don't think we are ready to add a feature like prefixing metric names more generally.
Since a lot of this is unknown, I would lean toward not exposing things publicly or building a lot of expectations right 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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818207509
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,15 +120,22 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
-
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
+
+ ImmutableMap<String, String> metricContextProperties;
Review comment:
should never use concrete class for map as the type.
Also can just use `Map<String, String> metricContextProperties = properties` and use a single if statement.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818208830
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -43,14 +45,15 @@
*/
public class S3FileIO implements FileIO {
private static final Logger LOG = LoggerFactory.getLogger(S3FileIO.class);
- private static final String DEFAULT_METRICS_IMPL = "org.apache.iceberg.hadoop.HadoopMetricsContext";
private SerializableSupplier<S3Client> s3;
private AwsProperties awsProperties;
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private static final String S3 = "s3";
Review comment:
variable name should be more specific for the purpose of usage, like `IO_METRICS_SCHEME_DEFAULT`
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818237362
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,28 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
+ LOG.info("Loading custom MetricsContext implementation: {}", impl);
+ MetricsContext metricsContext;
+ try {
+ DynConstructors.Ctor<MetricsContext> ctor =
+ DynConstructors.builder(MetricsContext.class).impl(impl).buildChecked();
+ metricsContext = ctor.newInstance();
+ } catch (NoSuchMethodException | ClassCastException e) {
+ LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", impl, e);
+ return null;
+ }
+
+ configureHadoopConf(metricsContext, hadoopConf);
+
+ metricsContext.initialize(properties);
+ return metricsContext;
+ }
+
+
Review comment:
nit: extra newline
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818237447
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,15 +119,19 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
Review comment:
nit: this comment can be removed
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818877252
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io-metrics-impl";
Review comment:
sorry just noticed this should be `io.metrics-impl`, which follows `io.metrics-scheme` better, could you make a change?
--
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
[GitHub] [iceberg] rdblue commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1075776203
It sounds like we agree about not setting up metrics with dynamic config, so I'm going to go ahead and close this one. Thanks for working on it, @kunal0829!
--
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
[GitHub] [iceberg] rdblue commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062031477
What I was thinking is a mix-in interface for `FileIO` that allows the engine to set the metrics context. We could default to Hadoop since that's what happens with `HadoopFileIO` automatically, but if you want to get passed metrics directly then you can set the context:
```java
FileIO io = table.io();
if (io instanceof MetricsProducer) {
((MetricsProducer) io).setMetricsContext(engineContext);
}
```
What do you think, @jackye1995?
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818154409
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,15 +117,12 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
-
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
- }
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
+ properties.put(CatalogProperties.IO_METRICS_SCHEME, properties.getOrDefault(
Review comment:
we cannot guarantee the properties map is mutable. we should create a new copy to put the metrics scheme
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818155551
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,25 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties) {
Review comment:
this should also have the `Object hadoopConf` in the parameter like `loadFileIO`, we will pass in `null` in S3FileIO but other implementations might leverage 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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819797231
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
sounds like using a term `io.metrics.namespace` seems like a more generic solution? @danielcweeks
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818233428
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,15 +120,18 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
-
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
+
+ ImmutableMap<String, String> metricContextProperties = ImmutableMap.copyOf(properties);
+ if (!properties.containsKey(CatalogProperties.IO_METRICS_SCHEME)) {
+ metricContextProperties = ImmutableMap.<String, String>builder()
+ .putAll(properties)
+ .put(CatalogProperties.IO_METRICS_SCHEME, IO_METRICS_SCHEME_S3)
+ .build();
}
+
+ this.metrics = CatalogUtil.loadFileIOMetricsContext(metricsImpl, metricContextProperties, null);
Review comment:
nit: add an inline comment for null input:
```java
CatalogUtil.loadFileIOMetricsContext(metricsImpl, metricContextProperties, null /* no Hadoop config */);
```
--
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
[GitHub] [iceberg] rdblue commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1061291885
@danielcweeks and @jackye1995, are we sure that dynamic loading is appropriate here?
I don't think that this fits with where we've used dynamic loading in the past. Dynamic loading is used where we want to be able to customize the implementation from the catalog. For example, a catalog may dynamically choose GCS or S3 or another FileIO implementation based on the catalog settings and table details (like root location). But the metrics reporting that this affects are being passed back to a processing engine, which is determined by the environment.
What we've been discussing for Flink is to have some way to connect FileIO metrics to Flink jobs and tasks, which likely means passing in a MetricsContext from Flink. This conflicts with the idea of passing down configuration that defines it. It doesn't make sense to me that we might allow a catalog or table to override something that is engine-specific.
One thing that Iceberg has been fairly successful at so far is trying to put configuration in the right place. I think this is really similar: we should not use table or catalog configuration for engine concerns.
--
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
[GitHub] [iceberg] danielcweeks commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819177181
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,27 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
+ LOG.info("Loading custom MetricsContext implementation: {}", impl);
+ MetricsContext metricsContext;
+ try {
+ DynConstructors.Ctor<MetricsContext> ctor =
+ DynConstructors.builder(MetricsContext.class).impl(impl).buildChecked();
+ metricsContext = ctor.newInstance();
+ } catch (NoSuchMethodException | ClassCastException e) {
+ LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", impl, e);
+ return null;
Review comment:
Specifically `MetricsContext.nullMetrics()`
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Dynamic MetricsContext Initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818091240
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopMetricsContext.java
##########
@@ -49,7 +49,7 @@ public HadoopMetricsContext(String scheme) {
public void initialize(Map<String, String> properties) {
// FileIO has no specific implementation class, but Hadoop will
// still track and report for the provided scheme.
- this.scheme = properties.getOrDefault(SCHEME, scheme);
+ this.scheme = properties.getOrDefault(CatalogProperties.IO_METRICS_SCHEME, scheme);
Review comment:
I think we no longer need constructor `public HadoopMetricsContext(String scheme)`, we should use a no-arg constructor because we are already initializing the scheme here. The scheme should be passed in from S3FileIO.
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopMetricsContext.java
##########
@@ -49,7 +49,7 @@ public HadoopMetricsContext(String scheme) {
public void initialize(Map<String, String> properties) {
// FileIO has no specific implementation class, but Hadoop will
// still track and report for the provided scheme.
- this.scheme = properties.getOrDefault(SCHEME, scheme);
+ this.scheme = properties.getOrDefault(CatalogProperties.IO_METRICS_SCHEME, scheme);
Review comment:
I think we no longer need constructor `public HadoopMetricsContext(String scheme)`, we should use a no-arg constructor because we are already initializing the scheme here. The scheme should be passed in from S3FileIO through the properties
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Dynamic MetricsContext Initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818088187
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io-metrics-impl";
+ public static final String DEFAULT_METRICS_IMPL = "org.apache.iceberg.hadoop.HadoopMetricsContext";
Review comment:
nit: IO_METRICS_IMPL_DEFAULT
--
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
[GitHub] [iceberg] rdblue closed pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821184017
##########
File path: api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java
##########
@@ -29,6 +29,21 @@
* like thread safety and serialization.
*/
public interface MetricsContext extends Serializable {
+ MetricsContext NULL_METRICS = new MetricsContext() {
Review comment:
Don't really have a good place to put this... what about this?
```java
/**
* Utility method for producing no metrics.
*
* @return a non-recording metrics context
*/
static MetricsContext nullMetrics() {
return NullMetricsContext.INSTANCE;
}
class NullMetricsContext implements MetricsContext {
private static final MetricsContext INSTANCE = new NullMetricsContext();
@Override
public <T extends Number> Counter<T> counter(String name, Class<T> type, Unit unit) {
return new Counter<T>() {
@Override
public void increment() {
}
@Override
public void increment(T amount) {
}
};
}
}
```
--
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
[GitHub] [iceberg] rdblue commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062169188
> Correct, however the current HadoopMetricsContext seems to be doing this across all the engines, and this offers a unique benefit for HadoopFileIO users that they are able to see their FileIO metrics across all compute platforms aggregate through the Hadoop FileSystem interface.
This is just a convenient way to get metrics to engines that already use Hadoop. I think we all agree that we want to be able to extend this, but it is a reasonable default for now.
> think it still makes sense to have some sort of default behavior controlled through catalog level property, maybe in the name of io.metrics.default.type which is limited to a few specific types including hadoop and collectd, and io.metrics.default.namespace provides the namespace of the metrics.
I don't think we are at a point where there's a good argument for catalog-level configuration. Catalog configuration would be in conflict with what engines need and we would have to find a more complicated way to avoid this problem.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821222884
##########
File path: api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java
##########
@@ -29,6 +29,21 @@
* like thread safety and serialization.
*/
public interface MetricsContext extends Serializable {
+ MetricsContext NULL_METRICS = new MetricsContext() {
Review comment:
Yeah, exactly. I think that should live in a separate file, though.
What was wrong with using the anonymous class?
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062258595
> This is just a convenient way to get metrics to engines that already use Hadoop. I think we all agree that we want to be able to extend this, but it is a reasonable default for now.
Yes, in that case let's first try to add the `MetricsProducer` mixin interface. I agree the catalog-based solution is more like a workaround, let's just implement the right thing.
In this model, how would we eventually make this not just a S3FileIO specific implementation?
I am thinking about something like the following in SparkCatalog
```java
private Pair<Table, Long> load(Identifier ident) {
...
Table table;
try {
table = icebergCatalog.loadTable(namespaceAsIdent);
FileIO io = table.io();
if (io instanceof MetricsProducer) {
// Report Hadoop metrics by default, will add logic and option for SparkMetricsContext in the future when that is available
try {
DynConstructors.Ctor<MetricsContext> ctor =
DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
// how would scheme be set for ResolvingFileIO? should we take table.location() as input to constructor to derive the scheme?
MetricsContext metrics = ctor.newInstance(io.scheme());
metrics.initialize(properties);
} catch (NoSuchMethodException | ClassCastException e) {
LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
}
((MetricsProducer) io).setMetricsContext(metrics);
}
} catch (Exception ignored) {
// the namespace does not identify a table, so it cannot be a table with a snapshot selector
// throw the original exception
throw e;
}
...
}
```
Would this make sense to you?
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818233756
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -43,14 +45,15 @@
*/
public class S3FileIO implements FileIO {
private static final Logger LOG = LoggerFactory.getLogger(S3FileIO.class);
- private static final String DEFAULT_METRICS_IMPL = "org.apache.iceberg.hadoop.HadoopMetricsContext";
private SerializableSupplier<S3Client> s3;
private AwsProperties awsProperties;
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private static final String IO_METRICS_SCHEME_S3 = "s3";
Review comment:
class variables should be placed before instance variables
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -43,14 +45,15 @@
*/
public class S3FileIO implements FileIO {
private static final Logger LOG = LoggerFactory.getLogger(S3FileIO.class);
- private static final String DEFAULT_METRICS_IMPL = "org.apache.iceberg.hadoop.HadoopMetricsContext";
private SerializableSupplier<S3Client> s3;
private AwsProperties awsProperties;
private transient S3Client client;
private MetricsContext metrics = MetricsContext.nullMetrics();
private final AtomicBoolean isResourceClosed = new AtomicBoolean(false);
+ private static final String IO_METRICS_SCHEME_S3 = "s3";
Review comment:
nit: class variables should be placed before instance variables
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818154837
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,15 +117,12 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
-
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
- }
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
+ properties.put(CatalogProperties.IO_METRICS_SCHEME, properties.getOrDefault(
+ CatalogProperties.IO_METRICS_SCHEME, "s3"));
Review comment:
s3 should be a static variable
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1057563888
@danielcweeks as we discussed in the community sync, this is the first step to add the metrics context support in other FileIOs, just move things to `CatalogProperties` to initialize the metrics context like other dynamically loaded classes, any comments?
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821223988
##########
File path: core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
##########
@@ -158,6 +160,26 @@ public void loadCustomFileIO_badClass() {
() -> CatalogUtil.loadFileIO(TestFileIONotImpl.class.getName(), Maps.newHashMap(), null));
}
+ @Test public void loadFileIOMetricsContext() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.IO_METRICS_NAMESPACE, "testScheme");
+ MetricsContext metricsContext = CatalogUtil.loadFileIOMetricsContext(
+ TestFileIOMetricsContext.class.getName(), properties, null);
+ TestFileIOMetricsContext testMetricsContext = (TestFileIOMetricsContext) metricsContext;
+
+ Assertions.assertThat(metricsContext).isInstanceOf(TestFileIOMetricsContext.class);
+ Assert.assertEquals("testScheme", testMetricsContext.scheme());
+ }
+
+ @Test public void loadBadFileIOMetricsContext() {
Review comment:
Annotations should be on the previous line.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819183477
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
I think there still needs to be a way for users to input some sort of grouping concept in the metrics, for example users who use `S3FileIO` for `gs://` should expect metrics to land to some different metrics namespace from `s3://`. Scheme seems to be a good way to do that. For our CloudWatch metrics integration. we will match this to the CloudWatch namespace concept, where the metrics will by default be emitted to a namepace `iceberg/<scheme>` such as `iceberg/s3`
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819182139
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,27 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
+ LOG.info("Loading custom MetricsContext implementation: {}", impl);
+ MetricsContext metricsContext;
+ try {
+ DynConstructors.Ctor<MetricsContext> ctor =
+ DynConstructors.builder(MetricsContext.class).impl(impl).buildChecked();
+ metricsContext = ctor.newInstance();
+ } catch (NoSuchMethodException | ClassCastException e) {
+ LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", impl, e);
+ return null;
Review comment:
yes good point I missed that, this should not return null
--
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
[GitHub] [iceberg] kunal0829 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
kunal0829 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819201759
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
Both statsd and prometheus it is called namespace. Collectd takes a combination of host and plugin to create a "namespace".
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r820932026
##########
File path: api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java
##########
@@ -29,6 +29,21 @@
* like thread safety and serialization.
*/
public interface MetricsContext extends Serializable {
+ MetricsContext NULL_METRICS = new MetricsContext() {
Review comment:
nvm, it's in an interface, cannot be private static final
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821065244
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -116,16 +118,19 @@ public void initialize(Map<String, String> properties) {
this.s3 = AwsClientFactories.from(properties)::s3;
}
- // Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
+ Map<String, String> metricContextProperties = properties;
+ if (!properties.containsKey(CatalogProperties.IO_METRICS_SCHEME)) {
+ metricContextProperties = ImmutableMap.<String, String>builder()
+ .putAll(properties)
+ .put(CatalogProperties.IO_METRICS_SCHEME, IO_METRICS_SCHEME_S3)
+ .build();
}
+
+ this.metrics = CatalogUtil.loadFileIOMetricsContext(
Review comment:
loadFileIOMetricsContext will now return `nullMetrics()` if cannot initialize Hadoop metrics context, so we should be fine 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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821224853
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_NAMESPACE = "io.metrics.namespace";
Review comment:
What about naming this more directly, `io.scheme` or `io.location-scheme`?
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Dynamic MetricsContext Initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818088982
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,14 +117,16 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
+ String metricsImpl = properties.getOrDefault(
Review comment:
can we move this chunk of initialization logic to `CatalogUtil` next to `loadFileIO`? Something like
```
CatalogUtil.loadFileIOMetricsContext(String impl,
Map<String, String> properties,
Object hadoopConf)
```
and scheme can be passed in as a part of the properties for initialization
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818234157
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,15 +120,18 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
-
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
+
+ ImmutableMap<String, String> metricContextProperties = ImmutableMap.copyOf(properties);
Review comment:
nit: no need to do copy of, use `Map<String, String>`
--
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
[GitHub] [iceberg] rdblue commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1075774762
I think the `MetricsProducer` interface that carries `setMetricsContext` makes sense. We may not even need anything in `Catalog`. I think it is likely that the Flink implementation would create a `MetricsContext` in each task and set it on the IO when setting up execution. The dynamic Hadoop handling is a good default, but we probably only need to expose a way to set 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
[GitHub] [iceberg] kunal0829 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
kunal0829 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819208834
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,27 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
+ LOG.info("Loading custom MetricsContext implementation: {}", impl);
+ MetricsContext metricsContext;
+ try {
+ DynConstructors.Ctor<MetricsContext> ctor =
+ DynConstructors.builder(MetricsContext.class).impl(impl).buildChecked();
+ metricsContext = ctor.newInstance();
+ } catch (NoSuchMethodException | ClassCastException e) {
+ LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", impl, e);
+ return null;
Review comment:
Thank you for that. I have fixed 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
[GitHub] [iceberg] danielcweeks commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819179732
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -116,16 +118,19 @@ public void initialize(Map<String, String> properties) {
this.s3 = AwsClientFactories.from(properties)::s3;
}
- // Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
+ Map<String, String> metricContextProperties = properties;
+ if (!properties.containsKey(CatalogProperties.IO_METRICS_SCHEME)) {
+ metricContextProperties = ImmutableMap.<String, String>builder()
+ .putAll(properties)
+ .put(CatalogProperties.IO_METRICS_SCHEME, IO_METRICS_SCHEME_S3)
+ .build();
}
+
+ this.metrics = CatalogUtil.loadFileIOMetricsContext(
Review comment:
this overrides the default `MetricsContext.nullMetrics()` that gets set in the variable initialization and will result in NPE (see other note in catalog util)
--
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
[GitHub] [iceberg] danielcweeks commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819176474
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,27 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
+ LOG.info("Loading custom MetricsContext implementation: {}", impl);
+ MetricsContext metricsContext;
+ try {
+ DynConstructors.Ctor<MetricsContext> ctor =
+ DynConstructors.builder(MetricsContext.class).impl(impl).buildChecked();
+ metricsContext = ctor.newInstance();
+ } catch (NoSuchMethodException | ClassCastException e) {
+ LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", impl, e);
+ return null;
Review comment:
We should return the null metrics impl not `null` here because engines without hadoop (e.g. Flink) will then NPE in the S3FileIO.
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062027218
> But the metrics reporting that this affects are being passed back to a processing engine, which is determined by the environment.
Yes I agree this is more related to the processing engine than catalog, rather than the catalog. The question here becomes how do we support integration to different engines without dynamically loading? We do not really have a way of configuring FileIO outside catalog.
The issue with HadoopMetricsContext is that we are going back to be tied to Hadoop. We have seen users trying to use Iceberg environments without Hadoop dependency (e.g. AWS Kinesis serverless Flink) and people don't want to load Hadoop dependencies just for getting metrics. So I think we definitely need some other implementations.
Our plan with this PR is to later introduce metrics context such as `CollectdMetricsContext`, and that could be then tied to any monitoring services that typically has an agent process running to collect metrics at background. Our target is AWS CloudWatch integration, but I think most products offer similar integration pattern.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821224853
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_NAMESPACE = "io.metrics.namespace";
Review comment:
I think the best solution here is to not define a public property for "namespace". I think the IO classes should just pass a "scheme" option into the metrics class. No need to define a contract or to try to make this generic.
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819183477
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
I think there still needs to be a way for users to input some sort of grouping concept in the metrics for non-Hadoop implementations, for example users who use `S3FileIO` for `gs://` should expect metrics to land to some different metrics namespace from `s3://`. Scheme seems to be a good way to do that.
For our CloudWatch metrics integration. we will match this to the CloudWatch namespace concept, where the metrics will by default be emitted to a namepace `iceberg/<scheme>` such as `iceberg/s3`
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821184233
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,27 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
+ LOG.info("Loading custom MetricsContext implementation: {}", impl);
+ MetricsContext metricsContext;
+ try {
+ DynConstructors.Ctor<MetricsContext> ctor =
+ DynConstructors.builder(MetricsContext.class).impl(impl).buildChecked();
+ metricsContext = ctor.newInstance();
+ } catch (NoSuchMethodException | ClassCastException e) {
+ LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", impl, e);
+ metricsContext = MetricsContext.nullMetrics();
Review comment:
nit: can directly return null metrics
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821156892
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -116,16 +118,19 @@ public void initialize(Map<String, String> properties) {
this.s3 = AwsClientFactories.from(properties)::s3;
}
- // Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
Review comment:
Looks like indentation is off in this PR. Can you fix 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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r820927277
##########
File path: api/src/main/java/org/apache/iceberg/metrics/MetricsContext.java
##########
@@ -29,6 +29,21 @@
* like thread safety and serialization.
*/
public interface MetricsContext extends Serializable {
+ MetricsContext NULL_METRICS = new MetricsContext() {
Review comment:
this should be private static final
--
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
[GitHub] [iceberg] jackye1995 edited a comment on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 edited a comment on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062027218
> But the metrics reporting that this affects are being passed back to a processing engine, which is determined by the environment.
Yes I agree this is more related to the processing engine, rather than the catalog. The question here becomes how do we support integration to different engines without dynamically loading? We do not really have a way of configuring FileIO outside catalog.
The issue with HadoopMetricsContext is that we are going back to be tied to Hadoop. We have seen users trying to use Iceberg environments without Hadoop dependency (e.g. AWS Kinesis serverless Flink) and people don't want to load Hadoop dependencies just for getting metrics. So I think we definitely need some other implementations.
Our plan with this PR is to later introduce metrics context such as `CollectdMetricsContext`, and that could be then tied to any monitoring services that typically has an agent process running to collect metrics at background. Our target is AWS CloudWatch integration, but I think most products offer similar integration pattern.
--
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
[GitHub] [iceberg] rdblue edited a comment on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
rdblue edited a comment on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062031477
What I was thinking is a mix-in interface for `FileIO` that allows the engine to set the metrics context. We could default to Hadoop since that's what happens with `HadoopFileIO` automatically, but if you want to get passed metrics directly then you can set the context:
```java
FileIO io = table.io();
if (io instanceof MetricsProducer) {
((MetricsProducer) io).setMetricsContext(engineContext);
}
```
What do you think, @jackye1995?
For reporting metrics to CloudWatch, wouldn't you integrate with the engine rather than with low-level libraries? You certainly could integrate directly if you wanted to. But I thought you'd probably want full metrics from the engine, not just what you can get from Iceberg file interaction.
--
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
[GitHub] [iceberg] danielcweeks commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819178970
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
`io.metrics-scheme` is hadoop specific since it's tied to a hadoop filesystem scheme in terms of reporting. Do we really need to move it here?
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -116,16 +118,19 @@ public void initialize(Map<String, String> properties) {
this.s3 = AwsClientFactories.from(properties)::s3;
}
- // Report Hadoop metrics if Hadoop is available
- try {
- DynConstructors.Ctor<MetricsContext> ctor =
- DynConstructors.builder(MetricsContext.class).hiddenImpl(DEFAULT_METRICS_IMPL, String.class).buildChecked();
- this.metrics = ctor.newInstance("s3");
+ String metricsImpl = properties.getOrDefault(
+ CatalogProperties.IO_METRICS_IMPL, CatalogProperties.IO_METRICS_IMPL_DEFAULT);
- metrics.initialize(properties);
- } catch (NoSuchMethodException | ClassCastException e) {
- LOG.warn("Unable to load metrics class: '{}', falling back to null metrics", DEFAULT_METRICS_IMPL, e);
+ Map<String, String> metricContextProperties = properties;
+ if (!properties.containsKey(CatalogProperties.IO_METRICS_SCHEME)) {
+ metricContextProperties = ImmutableMap.<String, String>builder()
+ .putAll(properties)
+ .put(CatalogProperties.IO_METRICS_SCHEME, IO_METRICS_SCHEME_S3)
+ .build();
}
+
+ this.metrics = CatalogUtil.loadFileIOMetricsContext(
Review comment:
this overrides the default `MetricsContext.nullMetrics()` that gets set in the variable initialization win will result in NPE (see other note in catalog util)
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818210797
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,26 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
Review comment:
please add a test for 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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818232644
##########
File path: core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
##########
@@ -402,4 +416,24 @@ public String getArg() {
public TestFileIONotImpl() {
}
}
+
+ public static class TestFileIOMetricsContext implements FileIOMetricsContext {
+ private String scheme;
+
+ public TestFileIOMetricsContext() {
+ }
+
+ @Override
+ public void initialize(Map<String, String> properties) {
+ // FileIO has no specific implementation class, but Hadoop will
Review comment:
nit: remove unnecessary stuffs for test usage, like L428, 429, 431, 432
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Dynamic MetricsContext Initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818088982
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,14 +117,16 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
+ String metricsImpl = properties.getOrDefault(
Review comment:
can we move this chunk of initialization logic to `CatalogUtil` next to `loadFileIO`?
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818155813
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopMetricsContext.java
##########
@@ -33,23 +34,18 @@
* statistics implementation using the provided scheme.
*/
public class HadoopMetricsContext implements FileIOMetricsContext {
- public static final String SCHEME = "io.metrics-scheme";
private String scheme;
private transient FileSystem.Statistics statistics;
- public HadoopMetricsContext(String scheme) {
- ValidationException.check(scheme != null,
- "Scheme is required for Hadoop FileSystem metrics reporting");
-
- this.scheme = scheme;
+ public HadoopMetricsContext() {
}
@Override
public void initialize(Map<String, String> properties) {
// FileIO has no specific implementation class, but Hadoop will
// still track and report for the provided scheme.
- this.scheme = properties.getOrDefault(SCHEME, scheme);
+ this.scheme = properties.get(CatalogProperties.IO_METRICS_SCHEME);
Review comment:
assert not null
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818207887
##########
File path: core/src/main/java/org/apache/iceberg/CatalogUtil.java
##########
@@ -275,6 +276,26 @@ public static FileIO loadFileIO(
return fileIO;
}
+ public static MetricsContext loadFileIOMetricsContext(
+ String impl,
+ Map<String, String> properties,
+ Object hadoopConf) {
Review comment:
we need to check and initialize `hadoopConf` like in `loadFileIO` (L373)
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Dynamic MetricsContext Initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r818088982
##########
File path: aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIO.java
##########
@@ -117,14 +117,16 @@ public void initialize(Map<String, String> properties) {
}
// Report Hadoop metrics if Hadoop is available
+ String metricsImpl = properties.getOrDefault(
Review comment:
can we move this chunk of initialization logic to `CatalogUtil` next to `loadFileIO`? Something like `CatalogUtil.loadFileIOMetricsContext(String scheme)`
--
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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r819194690
##########
File path: core/src/main/java/org/apache/iceberg/CatalogProperties.java
##########
@@ -81,4 +81,8 @@ private CatalogProperties() {
public static final String APP_ID = "app-id";
public static final String USER = "user";
+ public static final String IO_METRICS_IMPL = "io.metrics-impl";
+ public static final String IO_METRICS_IMPL_DEFAULT = "org.apache.iceberg.hadoop.HadoopMetricsContext";
+ public static final String IO_METRICS_SCHEME = "io.metrics-scheme";
Review comment:
Maybe we can give a more generic name for this than `scheme`, something like `namespace`? @kunal0829 what is the terminology used in statsd and collectd for 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
[GitHub] [iceberg] jackye1995 commented on a change in pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on a change in pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#discussion_r821184525
##########
File path: core/src/test/java/org/apache/iceberg/TestCatalogUtil.java
##########
@@ -158,6 +160,26 @@ public void loadCustomFileIO_badClass() {
() -> CatalogUtil.loadFileIO(TestFileIONotImpl.class.getName(), Maps.newHashMap(), null));
}
+ @Test public void loadFileIOMetricsContext() {
+ Map<String, String> properties = Maps.newHashMap();
+ properties.put(CatalogProperties.IO_METRICS_NAMESPACE, "testScheme");
+ MetricsContext metricsContext = CatalogUtil.loadFileIOMetricsContext(
+ TestFileIOMetricsContext.class.getName(), properties, null);
+ TestFileIOMetricsContext testMetricsContext = (TestFileIOMetricsContext) metricsContext;
+
+ Assertions.assertThat(metricsContext).isInstanceOf(TestFileIOMetricsContext.class);
+ Assert.assertEquals("testScheme", testMetricsContext.scheme());
+ }
+
+ @Test public void loadBadFileIOMetricsContext() {
+ Map<String, String> properties = Maps.newHashMap();
+ MetricsContext metricsContext = CatalogUtil.loadFileIOMetricsContext(
+ "BadMetricsContext", properties, null);
+
+ MetricsContext nullMetricsContext = MetricsContext.nullMetrics();
Review comment:
nit: can directly do `Assert.assertEquals(MetricsContext.nullMetrics(), metricsContext);`
--
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
[GitHub] [iceberg] jackye1995 commented on pull request #4254: Core: support dynamic MetricsContext initialization
Posted by GitBox <gi...@apache.org>.
jackye1995 commented on pull request #4254:
URL: https://github.com/apache/iceberg/pull/4254#issuecomment-1062074207
> I was thinking is a mix-in interface
Yes that could work!
> wouldn't you integrate with the engine rather than with low-level libraries
Correct, however the current `HadoopMetricsContext` seems to be doing this across all the engines, and this offers a unique benefit for `HadoopFileIO` users that they are able to see their FileIO metrics across all compute platforms aggregate through the Hadoop FileSystem interface.
There is benefit in at least providing an alternative such as `CollectdMetricsContext`, because we have been advocating `S3FileIO` to be not Hadoop dependent, and it is awkward that we now tell people we have to bring back the dependency and use Hadoop FileSystem to report metrics. This also applies to other non-Hadoop FileIOs.
Because of that, I think it still makes sense to have some sort of default behavior controlled through catalog level property, maybe in the name of `io.metrics.default.type` which is limited to a few specific types including `hadoop` and `collectd`, and `io.metrics.default.namespace` provides the namespace of the metrics.
(regarding metrics namespace, it was discussed in https://github.com/apache/iceberg/pull/4254#discussion_r819178970, it seems to be the terminology that most metrics system use, let me know what you think)
--
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