[FLINK-39130][metrics] Allow native types in MetricConfig#27642
[FLINK-39130][metrics] Allow native types in MetricConfig#27642Izeren wants to merge 1 commit intoapache:masterfrom
Conversation
| public int getInteger(String key, int defaultValue) { | ||
| String argument = getProperty(key, null); | ||
| return argument == null ? defaultValue : Integer.parseInt(argument); | ||
| final Object value = get(key); |
There was a problem hiding this comment.
I am looking at the the flip this looks like the last subtask to to in it.
But all the callers to MetricConfig will still call getString. It looks like we re missing another task to change the callers to call the appropriate native getter. Or better still add in the callers to this PR
There was a problem hiding this comment.
As part of the FLIP, I will introduce additional configs, like batch_size and my intention is to use it with getInt. I could include both changes in 1 PR as separate commits, however this change is rather a dependency than a part of the same "feature", so I have turned it into subtask on its own
| // -- String values via setProperty (legacy behavior) -- | ||
|
|
||
| @Test | ||
| void testGetIntegerFromString() { |
There was a problem hiding this comment.
I am wondering whether we could have a parameterized test for this. As the tests are basically the same for each native type.
There was a problem hiding this comment.
For defaults and strings, makes sense, will do. For "cross" not sure it is going to be more readable
There was a problem hiding this comment.
Actually, I have tried it out, and for cross it is still more readable, so I made all categories parametrized
Izeren
force-pushed
the
FLINK-39126/support-metric-export-batching
branch
from
183bee7 to
ba37c11
Compare
1996fanrui
left a comment
1996fanrui
left a comment
There was a problem hiding this comment.
Thanks for the fix, IIUC, this is a bug for all 2.x series, right?
If so, would you mind backporting it to all 2.x branch? thanks in advance.
4 participants
What is the purpose of the change
This allows using native types for the MetricConfig. Example usage is for FLIP-553 to allow configuring metric export batch size as: 1500 rather than "1500"
Brief change log
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation