[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3468 Good change, thanks! Merging this... --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3468 I think starting with `flink-tests` is good, we can then improve from there. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3468 So as I mentioned in JIRA I don't see a way to do it in checkstyle rule. I managed to implement a test that performs the check using `Reflections` but I don't know where shall I place it. The problem is that only files from packages that are dependencies to the module with this test will be checked. First idea I had was to put it in the `filnk-tests` module, but it does not depend on e.g. any of `flink-connectors` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3468 I would prefer to keep the value in `ConfigConstants`, just because it clearly defines in one place what we want to use as the default (rather than passing `StandardCharsets.UTF_8` everywhere). As for the name, I think both suggestions are fine in the end. It is unlikely that we ever switch the charset, especially since checkpoints and high-availability storage encodes data in that charset. What would be good to have is a test that checks that we never use a byte conversion without a charset. Maybe a checkstyle rule. If that does not work, we can try and do a `Reflections` test (see RpcCompletenessTest for an example of how to reflectively analyze code in tests). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user zentol commented on the issue: https://github.com/apache/flink/pull/3468 I'm in favor of using ```ConfigConstants.DEFAULT_CHARSET```. Using ```ConfigConstants.UTF_8``` does not imply in any way that this is the default that anyone should use; it would be equivalent to using ```StandardCharsets.UTF_8```. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user shijinkui commented on the issue: https://github.com/apache/flink/pull/3468 As your current replacement with `ConfigConstants.DEFAULT_CHARSET`, it's better for setting other charsets. UTF_8 has clear semantics. If we'll never change the default charset utf_8 to other charset, I prefer `ConfigConstants.UTF_8` --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/3468 @shijinkui I think the idea of having this Constant in `ConfigConstant` is it could potentially be set to some other value. If I am mistaken I would instead of using `ConfigConstants.UTF_8` opt for java7's `StandardCharsets.UTF_8` directly. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink issue #3468: [FLINK-5824] Fix String/byte conversions without explicit...
Github user shijinkui commented on the issue: https://github.com/apache/flink/pull/3468 Good changes. `ConfigConstants.DEFAULT_CHARSET` change to `ConfigConstants.UTF_8 ` may be more clear. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
