darenwkt commented on PR #22: URL: https://github.com/apache/flink-connector-prometheus/pull/22#issuecomment-2938983186
Thanks @hlteoh37 for the review! I have addressed the comments and appreciate if you could help take another look, thank you > 1/ There is currently tight coupling between `PrometheusSinkBase` and `PrometheusSink`, which is a code smell, of wrongly directed dependency! Given this is a relatively new connector, shall we consider instead taking the hit of making a breaking public interface change, and bumping the major version, to include only 1 `PrometheusSink` with the generic type? Yes, +1 to this, I have updated this in the latest revision > 2/ The current type handling for RowData -> String on metric labels is a little bit too open, in my opinion. https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/types/#list-of-data-types There is a fixed set of Flink SQL data types. We should consider fixing the list of available types for fields (e.g. CHAR, VARCHAR, etc) and rejecting types that are funky like (RAW, MULTISET etc.) This is a good callout, we should only support (CHAR, VARCHAR, STRING) for metric name and labels, I have updated this in the latest revision -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
