You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/04/12 14:04:08 UTC
[GitHub] [flink] zentol opened a new pull request, #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
zentol opened a new pull request, #19444:
URL: https://github.com/apache/flink/pull/19444
Based on #17228.
Metric reporters can currently be instantiated in one of 2 ways:
a) the reporter class is loaded via reflection
b) the reporter factory is loaded via reflection/ServiceLoader (aka, plugins)
All reporters provided by Flink use the factory approach, and it is preferable because it supports plugins. The plugin approach also has been available 1.11, and I think it's fair to deprecate the old approach by 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #19444:
URL: https://github.com/apache/flink/pull/19444#discussion_r853911263
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/ReporterSetup.java:
##########
@@ -360,6 +361,7 @@ private static List<ReporterSetup> setupReporters(
return reporterSetups;
}
+ @SuppressWarnings("deprecation")
private static Optional<MetricReporter> loadReporter(
Review Comment:
they generally don't have to touch reporters when upgrading. I made sure we log a warning whenever we don't use a factory.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #19444:
URL: https://github.com/apache/flink/pull/19444#discussion_r853933046
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/ReporterSetup.java:
##########
@@ -462,6 +462,15 @@ private static Optional<MetricReporter> loadViaReflection(
alternativeFactoryClassName);
return loadViaFactory(
alternativeFactoryClassName, reporterName, reporterConfig, reporterFactories);
+ } else {
+ LOG.warn(
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
zentol commented on code in PR #19444:
URL: https://github.com/apache/flink/pull/19444#discussion_r853907099
##########
flink-core/src/main/java/org/apache/flink/configuration/MetricOptions.java:
##########
@@ -67,8 +67,8 @@ public class MetricOptions {
+ " any of the names in the list will be started. Otherwise, all reporters that could be found in"
+ " the configuration will be started.");
- @Documentation.SuffixOption(NAMED_REPORTER_CONFIG_PREFIX)
- @Documentation.Section(value = Documentation.Sections.METRIC_REPORTERS, position = 1)
+ /** @deprecated use {@link MetricOptions#REPORTER_FACTORY_CLASS} instead. */
+ @Deprecated
public static final ConfigOption<String> REPORTER_CLASS =
Review Comment:
they are both deprecated now, and are effectively the same thing. I think the current path is fine; someone using `METRICS_REPORTER_CLASS_SUFFIX` is lead to `REPORTER_CLASS`, which then leads to `REPORTER_FACTORY_CLASS`.
Sure, we could remove 1 jump, but I don't think we'd really get anything out of 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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] XComp commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
XComp commented on code in PR #19444:
URL: https://github.com/apache/flink/pull/19444#discussion_r853878533
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/ReporterSetup.java:
##########
@@ -360,6 +361,7 @@ private static List<ReporterSetup> setupReporters(
return reporterSetups;
}
+ @SuppressWarnings("deprecation")
private static Optional<MetricReporter> loadReporter(
Review Comment:
What about adding a warn log message after the `factorClassName != null` if block stating that this option is deprecated to make this more visual to the user through logs? Or do users have to touch their reporter code anyway when updating Flink? Then, they would see it through the `@deprecated` annotation of the `@InterceptInstantiationViaReflection` annotation...
##########
flink-end-to-end-tests/flink-metrics-reporter-prometheus-test/src/test/java/org/apache/flink/metrics/prometheus/tests/PrometheusReporterEndToEndITCase.java:
##########
@@ -324,37 +296,28 @@ private static void checkMetricAvailability(final OkHttpClient client, final Str
static class TestParams {
private final String jarLocationDescription;
private final Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup;
- private final InstantiationType instantiationType;
private TestParams(
String jarLocationDescription,
- Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup,
- InstantiationType instantiationType) {
+ Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup) {
this.jarLocationDescription = jarLocationDescription;
this.builderSetup = builderSetup;
- this.instantiationType = instantiationType;
}
public static TestParams from(
String jarLocationDesription,
Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup,
InstantiationType instantiationType) {
- return new TestParams(jarLocationDesription, builderSetup, instantiationType);
+ return new TestParams(jarLocationDesription, builderSetup);
}
public Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> getBuilderSetup() {
return builderSetup;
}
- public InstantiationType getInstantiationType() {
- return instantiationType;
- }
-
@Override
public String toString() {
- return jarLocationDescription
- + ", instantiated via "
- + instantiationType.name().toLowerCase();
+ return jarLocationDescription;
}
public enum InstantiationType {
Review Comment:
The `REFLECTION` type is unused. I guess, we could get rid of the enum entirely now. No need to have this around...
##########
flink-end-to-end-tests/flink-metrics-reporter-prometheus-test/src/test/java/org/apache/flink/metrics/prometheus/tests/PrometheusReporterEndToEndITCase.java:
##########
@@ -324,37 +296,28 @@ private static void checkMetricAvailability(final OkHttpClient client, final Str
static class TestParams {
private final String jarLocationDescription;
private final Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup;
- private final InstantiationType instantiationType;
private TestParams(
String jarLocationDescription,
- Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup,
- InstantiationType instantiationType) {
+ Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup) {
this.jarLocationDescription = jarLocationDescription;
this.builderSetup = builderSetup;
- this.instantiationType = instantiationType;
}
public static TestParams from(
String jarLocationDesription,
Consumer<FlinkResourceSetup.FlinkResourceSetupBuilder> builderSetup,
InstantiationType instantiationType) {
Review Comment:
unused
##########
flink-core/src/main/java/org/apache/flink/configuration/MetricOptions.java:
##########
@@ -67,8 +67,8 @@ public class MetricOptions {
+ " any of the names in the list will be started. Otherwise, all reporters that could be found in"
+ " the configuration will be started.");
- @Documentation.SuffixOption(NAMED_REPORTER_CONFIG_PREFIX)
- @Documentation.Section(value = Documentation.Sections.METRIC_REPORTERS, position = 1)
+ /** @deprecated use {@link MetricOptions#REPORTER_FACTORY_CLASS} instead. */
+ @Deprecated
public static final ConfigOption<String> REPORTER_CLASS =
Review Comment:
[ConfigConstants#METRICS_REPORTER_CLASS_SUFFIX](https://github.com/apache/flink/blob/d834b271366ed508aba327aa94a14bb2b2e47a4c/flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java#L1116) is referring to this member. Just wanted to mention it but I guess it doesn't add any value...
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] flinkbot commented on pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
flinkbot commented on PR #19444:
URL: https://github.com/apache/flink/pull/19444#issuecomment-1096783029
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "af76914771ef4923660e43c30b1eef449776b544",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "af76914771ef4923660e43c30b1eef449776b544",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* af76914771ef4923660e43c30b1eef449776b544 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] XComp commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
XComp commented on code in PR #19444:
URL: https://github.com/apache/flink/pull/19444#discussion_r853950240
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/ReporterSetup.java:
##########
@@ -375,6 +377,15 @@ private static Optional<MetricReporter> loadReporter(
}
if (reporterClassName != null) {
+ LOG.warn(
+ "The reporter configuration of '{}' configures the reporter class, which is a deprecated approach to configure reporters'."
Review Comment:
```suggestion
"The reporter configuration of '{}' configures the reporter class, which is a deprecated approach to configure reporters."
```
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] XComp commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
XComp commented on code in PR #19444:
URL: https://github.com/apache/flink/pull/19444#discussion_r853919505
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/ReporterSetup.java:
##########
@@ -462,6 +462,15 @@ private static Optional<MetricReporter> loadViaReflection(
alternativeFactoryClassName);
return loadViaFactory(
alternativeFactoryClassName, reporterName, reporterConfig, reporterFactories);
+ } else {
+ LOG.warn(
Review Comment:
This warning won't be printed if user still used `MetricOptions#REPORTER_CLASS` but has a matching factory class that can be used for `loadViaFactory`. I think we should move this log message up in the call stack and add it to `loadReporter` instead...
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [flink] zentol merged pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
Posted by GitBox <gi...@apache.org>.
zentol merged PR #19444:
URL: https://github.com/apache/flink/pull/19444
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org