m1a2st commented on code in PR #19050:
URL: https://github.com/apache/kafka/pull/19050#discussion_r2041132665
##########
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java:
##########
@@ -40,14 +47,46 @@ private Plugin(T instance, PluginMetricsImpl pluginMetrics)
{
this.pluginMetrics = Optional.ofNullable(pluginMetrics);
}
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param tagsSupplier supplier to retrieve the tags
+ * @return the plugin
+ */
+ public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
Supplier<Map<String, String>> tagsSupplier) {
+ PluginMetricsImpl pluginMetrics = null;
+ if (instance instanceof Monitorable && metrics != null) {
+ pluginMetrics = new PluginMetricsImpl(metrics, tagsSupplier.get());
+ ((Monitorable) instance).withPluginMetrics(pluginMetrics);
+ }
+ return new Plugin<>(instance, pluginMetrics);
+ }
+
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param key the value for the <code>config</code> tag
+ * @return the plugin
+ */
public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
String key) {
return wrapInstance(instance, metrics, () -> tags(key, instance));
}
- public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
String key, Map<String, String> extraTags) {
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param name extra tag name to add
+ * @param value extra tag value to add
+ * @param key the value for the <code>config</code> tag
+ * @return the plugin
+ */
+ public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
String key, String name, String value) {
Review Comment:
Would it be better to use a `LinkedHashMap` instead of separate String
name/value pairs?
##########
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java:
##########
@@ -40,14 +47,46 @@ private Plugin(T instance, PluginMetricsImpl pluginMetrics)
{
this.pluginMetrics = Optional.ofNullable(pluginMetrics);
}
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
Review Comment:
ditto I think we should also mention that the instance's class name will be
used as the value for the `<code>class</code>` tag.
##########
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java:
##########
@@ -40,14 +47,46 @@ private Plugin(T instance, PluginMetricsImpl pluginMetrics)
{
this.pluginMetrics = Optional.ofNullable(pluginMetrics);
}
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param tagsSupplier supplier to retrieve the tags
+ * @return the plugin
+ */
+ public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
Supplier<Map<String, String>> tagsSupplier) {
+ PluginMetricsImpl pluginMetrics = null;
+ if (instance instanceof Monitorable && metrics != null) {
+ pluginMetrics = new PluginMetricsImpl(metrics, tagsSupplier.get());
+ ((Monitorable) instance).withPluginMetrics(pluginMetrics);
+ }
+ return new Plugin<>(instance, pluginMetrics);
+ }
+
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param key the value for the <code>config</code> tag
+ * @return the plugin
+ */
public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
String key) {
return wrapInstance(instance, metrics, () -> tags(key, instance));
}
- public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
String key, Map<String, String> extraTags) {
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
Review Comment:
I think we should also mention that the instance's class name will be used
as the value for the <code>class</code> tag.
##########
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java:
##########
@@ -40,14 +47,46 @@ private Plugin(T instance, PluginMetricsImpl pluginMetrics)
{
this.pluginMetrics = Optional.ofNullable(pluginMetrics);
}
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param tagsSupplier supplier to retrieve the tags
+ * @return the plugin
+ */
+ public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
Supplier<Map<String, String>> tagsSupplier) {
+ PluginMetricsImpl pluginMetrics = null;
+ if (instance instanceof Monitorable && metrics != null) {
+ pluginMetrics = new PluginMetricsImpl(metrics, tagsSupplier.get());
+ ((Monitorable) instance).withPluginMetrics(pluginMetrics);
+ }
+ return new Plugin<>(instance, pluginMetrics);
+ }
+
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
Review Comment:
ditto I think we should also mention that the instance's class name will be
used as the value for the `<code>class</code>` tag.
##########
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java:
##########
@@ -40,14 +47,46 @@ private Plugin(T instance, PluginMetricsImpl pluginMetrics)
{
this.pluginMetrics = Optional.ofNullable(pluginMetrics);
}
+ /**
+ * Wrap an instance into a Plugin.
+ * @param instance the instance to wrap
+ * @param metrics the metrics
+ * @param tagsSupplier supplier to retrieve the tags
+ * @return the plugin
+ */
+ public static <T> Plugin<T> wrapInstance(T instance, Metrics metrics,
Supplier<Map<String, String>> tagsSupplier) {
+ PluginMetricsImpl pluginMetrics = null;
+ if (instance instanceof Monitorable && metrics != null) {
+ pluginMetrics = new PluginMetricsImpl(metrics, tagsSupplier.get());
+ ((Monitorable) instance).withPluginMetrics(pluginMetrics);
+ }
+ return new Plugin<>(instance, pluginMetrics);
+ }
Review Comment:
Also ask the method PluginMetricsImpl#metricName, it also use
`Map.putAll()`, Should we also update it for order?
```
@Override
public MetricName metricName(String name, String description, Map<String,
String> tags) {
if (closing) throw new IllegalStateException("This PluginMetrics
instance is closed");
for (String tagName : tags.keySet()) {
if (this.tags.containsKey(tagName)) {
throw new IllegalArgumentException("Cannot use " + tagName + "
as a tag name");
}
}
Map<String, String> metricsTags = new LinkedHashMap<>(this.tags);
metricsTags.putAll(tags);
return metrics.metricName(name, GROUP, description, metricsTags);
}
```
##########
clients/src/main/java/org/apache/kafka/common/internals/Plugin.java:
##########
@@ -60,6 +99,13 @@ private static <T> Map<String, String> tags(String key, T
instance) {
return tags;
}
+ /**
+ * Wrap a list of instances into Plugins.
+ * @param instances the instances to wrap
Review Comment:
ditto I think we should also mention that the instance's class name will be
used as the value for the `<code>class</code>` tag.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
