zentol commented on a change in pull request #16645:
URL: https://github.com/apache/flink/pull/16645#discussion_r681007628
##########
File path:
flink-metrics/flink-metrics-influxdb/src/test/java/org/apache/flink/metrics/influxdb/MeasurementInfoProviderTest.java
##########
@@ -56,9 +78,26 @@ public void simpleTestGetMetricInfo() {
assertEquals(
String.join("" + MeasurementInfoProvider.SCOPE_SEPARATOR,
logicalScope, metricName),
info.getName());
- assertThat(info.getTags(), hasEntry("A", "a"));
- assertThat(info.getTags(), hasEntry("B", "b"));
- assertThat(info.getTags(), hasEntry("C", "c"));
+ assertThat(info.getTags(), hasEntry(KEY1, VALUE1));
+ assertThat(info.getTags(), hasEntry(KEY2, VALUE2));
+ assertThat(info.getTags(), hasEntry(KEY3, VALUE3));
Review comment:
It is not intuitive why this actually works; it only does because the
implementation is removing characters instead of replacing them with others.
##########
File path:
flink-metrics/flink-metrics-influxdb/src/test/java/org/apache/flink/metrics/influxdb/MeasurementInfoProviderTest.java
##########
@@ -33,16 +38,33 @@
import static org.junit.Assert.assertThat;
/** Test for {@link MeasurementInfoProvider}. */
+@RunWith(Parameterized.class)
public class MeasurementInfoProviderTest extends TestLogger {
+
+ private static final String KEY1 = "A";
+ private static final String KEY2 = "B";
+ private static final String KEY3 = "C";
+ private static final String VALUE1 = "a";
+ private static final String VALUE2 = "b";
+ private static final String VALUE3 = "c";
+
private final MeasurementInfoProvider provider = new
MeasurementInfoProvider();
+ private final boolean isDelimited;
+
+ @Parameters(name = "isDelimited = {0}")
+ public static List<Boolean> parameters() {
+ return Arrays.asList(false, true);
+ }
+
+ public MeasurementInfoProviderTest(boolean isDelimited) {
+ this.isDelimited = isDelimited;
+ }
+
@Test
public void simpleTestGetMetricInfo() {
String logicalScope = "myService.Status.JVM.ClassLoader";
- Map<String, String> variables = new HashMap<>();
- variables.put("<A>", "a");
- variables.put("<B>", "b");
- variables.put("<C>", "c");
Review comment:
please add a _dedicated_ test for the removal of newline characters in
variables. This test already does more than enough.
##########
File path:
flink-metrics/flink-metrics-influxdb/src/main/java/org/apache/flink/metrics/influxdb/MeasurementInfoProvider.java
##########
@@ -49,10 +50,13 @@ public MeasurementInfo getMetricInfo(String metricName,
MetricGroup group) {
private static Map<String, String> getTags(MetricGroup group) {
// Keys are surrounded by brackets: remove them, transforming "<name>"
to "name".
- Map<String, String> tags = new HashMap<>();
- for (Map.Entry<String, String> variable :
group.getAllVariables().entrySet()) {
+ Map<String, String> variables = group.getAllVariables();
+ Map<String, String> tags = new HashMap<>(variables.size() * 4 / 3 + 1);
+ for (Map.Entry<String, String> variable : variables.entrySet()) {
Review comment:
I would revert this; setting the default size of maps is the kind of
obtuse micro optimizations that we don't do.
##########
File path:
flink-metrics/flink-metrics-influxdb/src/test/java/org/apache/flink/metrics/influxdb/MeasurementInfoProviderTest.java
##########
@@ -56,9 +78,26 @@ public void simpleTestGetMetricInfo() {
assertEquals(
String.join("" + MeasurementInfoProvider.SCOPE_SEPARATOR,
logicalScope, metricName),
info.getName());
- assertThat(info.getTags(), hasEntry("A", "a"));
- assertThat(info.getTags(), hasEntry("B", "b"));
- assertThat(info.getTags(), hasEntry("C", "c"));
+ assertThat(info.getTags(), hasEntry(KEY1, VALUE1));
+ assertThat(info.getTags(), hasEntry(KEY2, VALUE2));
+ assertThat(info.getTags(), hasEntry(KEY3, VALUE3));
assertEquals(3, info.getTags().size());
}
+
+ private static Map<String, String> generateVariables(boolean isDelimited) {
+ Map<String, String> variables = new HashMap<>();
+ variables.put(String.format("<%s>", make(KEY1, isDelimited)),
make(VALUE1, isDelimited));
+ variables.put(String.format("<%s>", make(KEY2, isDelimited)),
make(VALUE2, isDelimited));
+ variables.put(String.format("<%s>", make(KEY3, isDelimited)),
make(VALUE3, isDelimited));
+ return variables;
+ }
+
+ private static String make(String value, boolean isDelimited) {
+ return isDelimited ? delimit(value) : value;
Review comment:
This is all overly complicated.
All we need is one test that does this:
```
variables = ("key\n", "value\n");
metricGroup = ...
info = provider.getMetricInfo(...)
assertThat(info.getTags(), containsEntry(key, value));
```
##########
File path:
flink-metrics/flink-metrics-influxdb/src/main/java/org/apache/flink/metrics/influxdb/MeasurementInfoProvider.java
##########
@@ -65,4 +69,8 @@ private static String getLogicalScope(MetricGroup group) {
return LogicalScopeProvider.castFrom(group)
.getLogicalScope(CHARACTER_FILTER, SCOPE_SEPARATOR);
}
+
+ private static String normalize(String value) {
+ return value != null ? value.replace(POINT_DELIMITER, "") : null;
Review comment:
the null check should be unnecessary.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
