You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by yanghua <gi...@git.apache.org> on 2018/05/15 07:40:30 UTC
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
GitHub user yanghua opened a pull request:
https://github.com/apache/flink/pull/6016
[FLINK-8933] Avoid calling Class#newInstance(part 2)
## What is the purpose of the change
*This pull request avoid calling Class#newInstance*
## Brief change log
- *Replace `Class#newInstance` to `Class#getDeclaredConstructor#newInstance`*
## Verifying this change
This change is already covered by existing tests*.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): (yes / **no**)
- The public API, i.e., is any changed class annotated with `@Public(Evolving)`: (yes / **no**)
- The serializers: (yes / **no** / don't know)
- The runtime per-record code paths (performance sensitive): (yes / **no** / don't know)
- Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / **no** / don't know)
- The S3 file system connector: (yes / **no** / don't know)
## Documentation
- Does this pull request introduce a new feature? (yes / **no**)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / **not documented**)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/yanghua/flink FLINK-8933-1
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/6016.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 #6016
----
commit b6408814ac4b29c182ec1f1ed3ea1788fbc7097b
Author: yanghua <ya...@...>
Date: 2018-05-15T07:36:10Z
[FLINK-8933] Avoid calling Class#newInstance(part 2)
----
---
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6016#discussion_r188246145
--- Diff: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/TupleSerializer.java ---
@@ -76,7 +76,7 @@ public T createInstance() {
public T createInstance(Object[] fields) {
try {
- T t = tupleClass.newInstance();
+ T t = tupleClass.getDeclaredConstructor().newInstance();
--- End diff --
this change may have performance implications as noted in the mail thread linked in the jira.
---
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6016#discussion_r188246129
--- Diff: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/TupleSerializer.java ---
@@ -59,7 +59,7 @@ public TupleSerializer(Class<T> tupleClass, TypeSerializer<?>[] fieldSerializers
@Override
public T createInstance() {
try {
- T t = tupleClass.newInstance();
+ T t = tupleClass.getDeclaredConstructor().newInstance();
--- End diff --
this change may have performance implications as noted in the mail thread linked in the jira.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6016
Should we close this PR until we have consensus whether we want to change this?
Especially the performance implications in methods/classes on the "hot code paths" makes this a tricky change...
---
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6016#discussion_r188246102
--- Diff: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java ---
@@ -225,7 +225,7 @@ public T copy(T from) {
if (actualType == clazz) {
T target;
try {
- target = (T) from.getClass().newInstance();
+ target = (T) from.getClass().getDeclaredConstructor().newInstance();
--- End diff --
this change may have performance implications as noted in the mail thread linked in the jira.
---
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua closed the pull request at:
https://github.com/apache/flink/pull/6016
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6016
agree, closing this...
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6016
I am not yet convinced that this fixes an actual problem, whereas it is not clear whether it may cause regression (exception traces or performance).
To me this is a case to not do this change, but spend effort on other issues.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6016
I mean the issue was thrown before the review, that may be considered it was not a principal problem, deprecating `Class.newInstance` most reason is used to fix undeclared checked exceptions (even if there is a little performance cost). And I think JDK's API developer should not make hasty decision. Maybe we can watch the JDK's implementation about both of them with IDE.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/6016
hmm, according to the [javadocs](https://docs.oracle.com/javase/7/docs/api/java/lang/reflect/InvocationTargetException.html) the wrapped exception is part of the exception chain and accessible via `getCause()`. Can't try it out mself at the moment though.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/6016
I am not sure we should be doing it like this.
The point of discouraging `Class.newInstance()` is not to copy replace this by a different call. That will not magically make anything better. The whole point is to go from a method that may sneakily throw undeclared exceptions, to a method that declares exceptions.
All places that do not require a change to the try/catch statement (add `InvokationTargetException`) have reasonable exception handling in place already, and are not vulnerable to the motivating issue.
In fact, for now this makes thing worse because Exceptions get wrapped into `InvokationTargetException`, with the root cause in the `getTargetException()`, not in the exception chain. That would actually be a serious regression.
As a more general comment: Copy/replace style pull requests are a bit tricky, in my opinion - they are rather easy to create (bulk fix tool) and require committers to put up significant time to dig into the matter and to double check that it actually does not cause a regression in the individual spots that are changed. One can probably keep all committers busy for a year with just such fixes without really changing anything for users for the better, and having a high risk of regressions.
I would personally vote to not have many of those pull requests, or clearly label them as "cosmetic" and give them a very low priority in reviewing. Especially given that we have a decent backlog of pull requests for changes that where users will see a difference.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/6016
and I look at the review threads as well, which apparently didn't address the issue.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6016
hi @zentol I find this pattern has been defined as a [BugPattern](https://github.com/google/error-prone/issues/407) in google's [error-prone](https://github.com/google/error-prone) which is a lib catches common Java mistakes as compile-time errors. And the link you provided is not the last result, there were [review items](https://bugs.openjdk.java.net/browse/JDK-6850612) after that discuss.
---
[GitHub] flink issue #6016: [FLINK-8933] Avoid calling Class#newInstance(part 2)
Posted by yanghua <gi...@git.apache.org>.
Github user yanghua commented on the issue:
https://github.com/apache/flink/pull/6016
OK, I agree @StephanEwen , let's give it a low priority. Actually, I know this big change can bring nondeterminacy to the codebase. So when I assigned this issue to myself, I asked your opinion in JIRA. But it does not matter, the discussion is significative.
Thanks @zentol started the discussion and preciseness about the change. I think we could discuss again about this issue, as I said before, google's error-prone also marked this as a bug pattern and JDK team is also preciseness before they gave this statement.
---
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6016#discussion_r188246084
--- Diff: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/PojoSerializer.java ---
@@ -193,7 +193,7 @@ public T createInstance() {
return null;
}
try {
- T t = clazz.newInstance();
+ T t = clazz.getDeclaredConstructor().newInstance();
--- End diff --
this change may have performance implications as noted in the mail thread linked in the jira.
---
[GitHub] flink pull request #6016: [FLINK-8933] Avoid calling Class#newInstance(part ...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/6016#discussion_r188246156
--- Diff: flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/TupleSerializer.java ---
@@ -150,7 +150,7 @@ public T deserialize(T reuse, DataInputView source) throws IOException {
private T instantiateRaw() {
try {
- return tupleClass.newInstance();
+ return tupleClass.getDeclaredConstructor().newInstance();
--- End diff --
this change may have performance implications as noted in the mail thread linked in the jira.
---