[GitHub] [flink] zentol commented on pull request #14510: [FLINK-19656] [metrics] filter delimiter in metrics components

[GitBox <gi...@apache.org>]

zentol commented on pull request #14510:
URL: https://github.com/apache/flink/pull/14510#issuecomment-762315443


   @echauchot I made a final pass over the PR and did some changes:
   
   1) I have removed the filtering of variables. This was my bad (again); filtering the delimiter from variables doesn't make a lot of sense since they are not used in a context where the delimiter usually matters. This could also have resulted in some unexpected behavior from the reporter side, since depending on the delimiter it would no longer be possible to easily look up the value for a specific variable. (image a reporter using '_' as the delimiter; this would change quite a few scope variables).
   Filtering variables could be a useful follow-up, but it's a bit tricky due to the lookup issue.
   2) Since we only want to do the filtering when used from a reporter I have moved the filter assembly into the `FrontMetricGroup`. I have kept the simplifications in the `AbstractMetricGroup` regarding filters being nullable, by enforcing that they are now not null.
   3) I have added a check whether the delimiter used clashes with the default replacement ('_'). If so we use '-' as a backup replacement.
   
   I apologize for wasting your time working to implement the variable filtering.
   Thank you for working on this rather nasty part of the codebase.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org