[FLINK-36946][Metrics] Optimize sink operator name truncate #25832
Conversation
There was a problem hiding this comment.
Thanks @Tartarus0zm for the optimization!
LGTM assuming CI is green.
| @@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) { | |||
| } | |||
| } | |||
|
|
|||
| public static String truncateOperatorName(String operatorName, int maxLength) { | |||
There was a problem hiding this comment.
could we have a unit test for this method please.
There was a problem hiding this comment.
if maxlength is less than the Suffix length and there is a suffix in the data, this will end up with a negative length. Or is this not possible?
There was a problem hiding this comment.
could we have a unit test for this method please.
good catch
There was a problem hiding this comment.
if maxlength is less than the Suffix length and there is a suffix in the data, this will end up with a negative length. Or is this not possible?
Thanks @davidradl for the comment, it's interesting. IIUC, current PR only has 2 suffixes: Writer and Committer. Both of length are less than the maxlength(80), so it doesn't happen.
But I'm not sure will it happen in the future. Anyway, we could optimize the logic to avoid the negative length. It will make the code more robust.
@Tartarus0zm WDYT?
BTW, it's better to let reviewer click the resolve conversation. It's helpful for reviewer to check whether all comments are addressed properly.
There was a problem hiding this comment.
@1996fanrui thanks for your remind. I has re-open the conversation.
Also, I've updated this PR, MetricUtils#truncateOperatorName internally does the length judgement of the OperatorName.
cc @davidradl
There was a problem hiding this comment.
Have you pushed your code to github repo? The PR isn't updated, the latest change was 2 weeks ago.
There was a problem hiding this comment.
@1996fanrui yes, I updated this PR two weeks ago.
a. optimized MetricUtils#truncateOperatorName, internal judgement of how OperatorName should be truncated.
b. add Test MetricUtilsTest.java for MetricUtils#truncateOperatorName.
There was a problem hiding this comment.
@1996fanrui @davidradl hello , I update this PR again.
please take a look, thanks
thanks for your review, I'll update the commit to addressing your concerns. |
|
@1996fanrui @davidradl hi, I has updated the commit, take a look again? |
What is the purpose of the change
Avoid truncation of the Writer/Committer keyword.
Brief change log
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation