Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/750#issuecomment-142970422
There are some crucial issues still with this pull request:
Most importantly, it changes parts that it needs not change, altering
system behavior. That manifests itself in the 130+ changed files, for something
that should probably touch around 20 files.
As per the contribution guidelines, it is very important that pull requests
do not do that. We have seen too many cases where bugs were introduced because
code parts were changed for no other reason than someone thinking that the code
should look different in their opinion.
Reporting and changing such unrelated parts should always be a separate
issue.
In the first few files, I see examples of
- Changing the type of exceptions caught. We often explicitly catch
Throwables, because we want to handle Errors as well, such as LinkageErrors
which are common when the dynamic class loading fails.
- Changing of the way tests deal with exceptions. There are good reasons
to explicitly catch and check expected exceptions (check messages, safe against
maven mis-configurations where exceptions propagating out of tests were not
correctly recognized, I saw that more than once so far).
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
