KAFKA-19139 Plugin#wrapInstance should use LinkedHashMap instead of Map #19519
Conversation
github-actions
bot
added
triage
|
Good. Using LinkedHashMap makes sense here to ensure consistent tag ordering and avoid duplicate time series in metrics backends. The trade-off is acceptable since this is not on a performance-critical path. |
There was a problem hiding this comment.
@m1a2st: Thanks for the patch.
Could you please check the modified files and eliminate the double brace initialization?
| private static final LinkedHashMap<String, String> TAGS = new LinkedHashMap<>() {{ | ||
| put("k", "v"); | ||
| }}; |
There was a problem hiding this comment.
nit: Could we use a static block instead of double brace initialization?
| Properties baseProps = new Properties() {{ | ||
| setProperty(ProducerConfig.BOOTSTRAP_SERVERS_CONFIG, "localhost:9999"); | ||
| setProperty(ProducerConfig.KEY_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); | ||
| setProperty(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, StringSerializer.class.getName()); | ||
| }}; |
There was a problem hiding this comment.
Could we also clean this, and do we need static here?
| @@ -36,11 +35,15 @@ | |||
|
|
|||
| public class PluginMetricsImplTest { | |||
|
|
|||
| private final Map<String, String> extraTags = Collections.singletonMap("my-tag", "my-value"); | |||
| private static final LinkedHashMap<String, String> EXTRA_TAGS = new LinkedHashMap<>(); | |||
There was a problem hiding this comment.
Could we eliminate the static keyword?
private final LinkedHashMap<String, String> EXTRA_TAGS = new LinkedHashMap<>(Map.of("my-tag", "my-value"));
| @@ -2881,9 +2875,13 @@ private MetricName expectedMetricName(String clientId, String config, Class<?> c | |||
|
|
|||
| private static final String NAME = "name"; | |||
| private static final String DESCRIPTION = "description"; | |||
| private static final Map<String, String> TAGS = Collections.singletonMap("k", "v"); | |||
| private static final LinkedHashMap<String, String> TAGS = new LinkedHashMap<>(); | |||
There was a problem hiding this comment.
private static final LinkedHashMap<String, String> TAGS = new LinkedHashMap<>(Map.of("t1", "v1"));
| @@ -64,10 +64,13 @@ public class ConnectMetricsTest { | |||
| WorkerConfig.KEY_CONVERTER_CLASS_CONFIG, "org.apache.kafka.connect.json.JsonConverter", | |||
| WorkerConfig.VALUE_CONVERTER_CLASS_CONFIG, "org.apache.kafka.connect.json.JsonConverter"); | |||
| private static final ConnectorTaskId CONNECTOR_TASK_ID = new ConnectorTaskId("connector", 0); | |||
| private static final Map<String, String> TAGS = Map.of("t1", "v1"); | |||
|
|
|||
| private static final LinkedHashMap<String, String> TAGS = new LinkedHashMap<>(); | |||
| assertEquals(0, metrics.metric(metricName).metricValue()); | ||
| monitorableAuthorizer.authorize(null, null); | ||
| assertEquals(1, metrics.metric(metricName).metricValue()); | ||
| } | ||
|
|
||
| public static class MonitorableAuthorizer implements Authorizer, Monitorable { | ||
|
|
||
| private final Map<String, String> extraTags = Map.of("k1", "v1"); | ||
| private static final LinkedHashMap<String, String> EXTRA_TAGS = new LinkedHashMap<>(); |
There will be an update to the PluginMetrics#metricName method: the type
of the tags parameter will be changed
from Map to LinkedHashMap.
This change is necessary because the order of metric tags is important
distinct ones by the metrics backend
consistency, we should follow the same approach here.