Uploaded image for project: 'Flink'
  1. Flink
  2. FLINK-6898

Limit size of operator component in metric name

        Details

        • Type: Improvement
        • Status: Closed
        • Priority: Critical
        • Resolution: Fixed
        • Affects Version/s: 1.3.0, 1.4.0
        • Fix Version/s: 1.4.0, 1.3.2
        • Component/s: Metrics
        • Labels:
          None

          Description

          The operator name for some operators (specifically windows) can be very, very long (250+) characters.

          I propose to limit the total space that the operator component can take up in a metric name to 60 characters.

            Issue Links

            links to

            Web Link GitHub Pull Request #4109

              Activity

              Descending order - Click to sort in ascending order
              Hide
              Permalink
              Zentol Chesnay Schepler added a comment -

              1.3: 1d2c615853ffa627a61de6c9880c53c3b00f1e31
              1.4: edb79b0a8bb2b33e1a0470b99b11274ac0e9c673

              Show
              Zentol Chesnay Schepler added a comment - 1.3: 1d2c615853ffa627a61de6c9880c53c3b00f1e31 1.4: edb79b0a8bb2b33e1a0470b99b11274ac0e9c673
              Hide
              Permalink
              githubbot ASF GitHub Bot added a comment -

              Github user asfgit closed the pull request at:

              https://github.com/apache/flink/pull/4109

              Show
              githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/4109
              Hide
              Permalink
              githubbot ASF GitHub Bot added a comment -

              Github user zentol commented on the issue:

              https://github.com/apache/flink/pull/4109

              a constant is a good idea, will add it and start merging.

              Show
              githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/4109 a constant is a good idea, will add it and start merging.
              Hide
              Permalink
              githubbot ASF GitHub Bot added a comment -

              Github user greghogan commented on the issue:

              https://github.com/apache/flink/pull/4109

              Update looks good. Do we want to define `80` as a constant?

              Show
              githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/4109 Update looks good. Do we want to define `80` as a constant?
              Hide
              Permalink
              githubbot ASF GitHub Bot added a comment -

              Github user zentol commented on the issue:

              https://github.com/apache/flink/pull/4109

              no, it's not safe nor does it actually fulfill the purpose. I better add a test for this....

              We can't just fail, as the metric system shouldn't throw exceptions, since this isn't a problem that affects job execution.

              I've moved the truncation into `TaskMetricGroup#addOperator()` which is the only call to the constructor outside of tests (as intended).

              Show
              githubbot ASF GitHub Bot added a comment - Github user zentol commented on the issue: https://github.com/apache/flink/pull/4109 no, it's not safe nor does it actually fulfill the purpose. I better add a test for this.... We can't just fail, as the metric system shouldn't throw exceptions, since this isn't a problem that affects job execution. I've moved the truncation into `TaskMetricGroup#addOperator()` which is the only call to the constructor outside of tests (as intended).
              Hide
              Permalink
              githubbot ASF GitHub Bot added a comment -

              Github user greghogan commented on the issue:

              https://github.com/apache/flink/pull/4109

              Is it safe to set the full `operatorName` in the parent constructor scope but truncate in the local info and variables map? Would it be better to explicitly fail rather than implicitly truncate?

              Show
              githubbot ASF GitHub Bot added a comment - Github user greghogan commented on the issue: https://github.com/apache/flink/pull/4109 Is it safe to set the full `operatorName` in the parent constructor scope but truncate in the local info and variables map? Would it be better to explicitly fail rather than implicitly truncate?
              Hide
              Permalink
              githubbot ASF GitHub Bot added a comment -

              GitHub user zentol opened a pull request:

              https://github.com/apache/flink/pull/4109

              FLINK-6898 [metrics] Limit size of operator component in metric name

              This PR prevents the operator name component of a metric name to exceed 80 characters. I picked 80 since it is below 255 (a common maximum for file lengths), but still large enough for that once-in-a-million special case. I guess.

              You can merge this pull request into a Git repository by running:

              $ git pull https://github.com/zentol/flink 6898

              Alternatively you can review and apply these changes as the patch at:

              https://github.com/apache/flink/pull/4109.patch

              To close this pull request, make a commit to your master/trunk branch
              with (at least) the following in the commit message:

              This closes #4109

              commit 71bf096d6562d45a3b235ad8c027cdd601f6d937
              Author: zentol <chesnay@apache.org>
              Date: 2017-06-12T13:36:25Z

              FLINK-6898 [metrics] Limit size of operator component in metric name

              Show
              githubbot ASF GitHub Bot added a comment - GitHub user zentol opened a pull request: https://github.com/apache/flink/pull/4109 FLINK-6898 [metrics] Limit size of operator component in metric name This PR prevents the operator name component of a metric name to exceed 80 characters. I picked 80 since it is below 255 (a common maximum for file lengths), but still large enough for that once-in-a-million special case. I guess. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zentol/flink 6898 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/4109.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #4109 commit 71bf096d6562d45a3b235ad8c027cdd601f6d937 Author: zentol <chesnay@apache.org> Date: 2017-06-12T13:36:25Z FLINK-6898 [metrics] Limit size of operator component in metric name

                People

                • Assignee:
                  Zentol Chesnay Schepler
                  Reporter:
                  Zentol Chesnay Schepler
                • Votes:
                  0 Vote for this issue
                  Watchers:
                  1 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:

                    Development