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/20 09:01:09 UTC
[GitHub] [flink] XComp commented on a diff in pull request #19444: [FLINK-27206][metrics] Deprecate reflection-based reporter instantiation
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