You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by "Tartarus0zm (via GitHub)" <gi...@apache.org> on 2024/12/20 12:12:30 UTC
[PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Tartarus0zm opened a new pull request, #25832:
URL: https://github.com/apache/flink/pull/25832
## What is the purpose of the change
Avoid truncation of the Writer/Committer keyword.
## Brief change log
- add MetricUtil#truncateOperatorName to optimize Operator name
## Verifying this change
- SinkMetricsITCase#testMetrics
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (no)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (no)
- The serializers: (no)
- The runtime per-record code paths (performance sensitive): (no)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
- The S3 file system connector: (no)
## Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not documented)
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "Tartarus0zm (via GitHub)" <gi...@apache.org>.
Tartarus0zm commented on PR #25832:
URL: https://github.com/apache/flink/pull/25832#issuecomment-2558388134
> see comments
thanks for your review, I'll update the commit to addressing your concerns.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "flinkbot (via GitHub)" <gi...@apache.org>.
flinkbot commented on PR #25832:
URL: https://github.com/apache/flink/pull/25832#issuecomment-2556895215
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "cee66261ba96fb8c49b4da0f74abc7c33c05b146",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "cee66261ba96fb8c49b4da0f74abc7c33c05b146",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* cee66261ba96fb8c49b4da0f74abc7c33c05b146 UNKNOWN
<details>
<summary>Bot commands</summary>
The @flinkbot bot supports the following commands:
- `@flinkbot run azure` re-run the last Azure build
</details>
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "Tartarus0zm (via GitHub)" <gi...@apache.org>.
Tartarus0zm commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1906747352
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
@1996fanrui @davidradl hello , I update this PR again.
please take a look, thanks
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1893941196
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review 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?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "Tartarus0zm (via GitHub)" <gi...@apache.org>.
Tartarus0zm commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1894854901
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
> could we have a unit test for this method please.
good catch
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "Tartarus0zm (via GitHub)" <gi...@apache.org>.
Tartarus0zm commented on PR #25832:
URL: https://github.com/apache/flink/pull/25832#issuecomment-2558390123
@1996fanrui @davidradl hi, I has updated the commit, take a look again?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui merged PR #25832:
URL: https://github.com/apache/flink/pull/25832
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "Tartarus0zm (via GitHub)" <gi...@apache.org>.
Tartarus0zm commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1904058841
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review 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
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1901545914
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review 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.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1893941196
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
if maxlength is less than the Suffix length and there is a suffix in the data, shouldn't this be an error as the 2 values cannot be reconciled?
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1904833412
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
Have you pushed your code to github repo? The PR isn't updated, the latest change was 2 weeks ago.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "davidradl (via GitHub)" <gi...@apache.org>.
davidradl commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1893938328
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
could we have a unit test for this method please.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "1996fanrui (via GitHub)" <gi...@apache.org>.
1996fanrui commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1904833412
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
Have you pushed your code to github repo? The PR isn't updated.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [FLINK-36946][Metrics] Optimize sink operator name truncate [flink]
Posted by "Tartarus0zm (via GitHub)" <gi...@apache.org>.
Tartarus0zm commented on code in PR #25832:
URL: https://github.com/apache/flink/pull/25832#discussion_r1904853898
##########
flink-runtime/src/main/java/org/apache/flink/runtime/metrics/util/MetricUtils.java:
##########
@@ -366,6 +369,17 @@ private static void instantiateCPUMetrics(MetricGroup metrics) {
}
}
+ public static String truncateOperatorName(String operatorName, int maxLength) {
Review Comment:
@1996fanrui yes, I updated this PR two weeks ago.
<img width="843" alt="image" src="https://github.com/user-attachments/assets/20a7a13a-2581-41c3-b674-24f60db98860" />
a. optimized MetricUtils#truncateOperatorName, internal judgement of how OperatorName should be truncated.
b. add Test [MetricUtilsTest.java](https://github.com/apache/flink/pull/25832/files#diff-3a4d0047f0aea26a1d175856b9242df15dcf524a01d6076955d5a5608245a8e1) for MetricUtils#truncateOperatorName.
--
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-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org