[GitHub] flink pull request: [FLINK-2437] Fix default constructor detection...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-130251642 Thanks for your contribution! --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/960 --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user ggevay commented on a diff in the pull request:
https://github.com/apache/flink/pull/960#discussion_r36843332
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
---
@@ -1328,24 +1329,29 @@ else if(typeHierarchy.size() <= 1) {
List methods = getAllDeclaredMethods(clazz);
for (Method method : methods) {
if (method.getName().equals("readObject") ||
method.getName().equals("writeObject")) {
- LOG.info("Class "+clazz+" contains custom
serialization methods we do not call.");
+ LOG.info(clazz+" contains custom serialization
methods we do not call.");
return null;
}
}
// Try retrieving the default constructor, if it does not have
one
// we cannot use this because the serializer uses it.
+ Constructor defaultConstructor = null;
try {
- clazz.getDeclaredConstructor();
+ defaultConstructor = clazz.getDeclaredConstructor();
} catch (NoSuchMethodException e) {
if (clazz.isInterface() ||
Modifier.isAbstract(clazz.getModifiers())) {
- LOG.info("Class " + clazz + " is abstract or an
interface, having a concrete " +
+ LOG.info(clazz + " is abstract or an interface,
having a concrete " +
"type can increase
performance.");
} else {
- LOG.info("Class " + clazz + " must have a
default constructor to be used as a POJO.");
+ LOG.info(clazz + " must have a default
constructor to be used as a POJO.");
return null;
}
}
+ if(defaultConstructor != null &&
(defaultConstructor.getModifiers() & Modifier.PUBLIC) == 0) {
--- End diff --
You are right, I have changed it.
---
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 pull request: [FLINK-2437] Fix default constructor detection...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-130219402 Your changes look good to merge. --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/960#discussion_r36837638
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
---
@@ -1328,24 +1329,29 @@ else if(typeHierarchy.size() <= 1) {
List methods = getAllDeclaredMethods(clazz);
for (Method method : methods) {
if (method.getName().equals("readObject") ||
method.getName().equals("writeObject")) {
- LOG.info("Class "+clazz+" contains custom
serialization methods we do not call.");
+ LOG.info(clazz+" contains custom serialization
methods we do not call.");
return null;
}
}
// Try retrieving the default constructor, if it does not have
one
// we cannot use this because the serializer uses it.
+ Constructor defaultConstructor = null;
try {
- clazz.getDeclaredConstructor();
+ defaultConstructor = clazz.getDeclaredConstructor();
} catch (NoSuchMethodException e) {
if (clazz.isInterface() ||
Modifier.isAbstract(clazz.getModifiers())) {
- LOG.info("Class " + clazz + " is abstract or an
interface, having a concrete " +
+ LOG.info(clazz + " is abstract or an interface,
having a concrete " +
"type can increase
performance.");
} else {
- LOG.info("Class " + clazz + " must have a
default constructor to be used as a POJO.");
+ LOG.info(clazz + " must have a default
constructor to be used as a POJO.");
return null;
}
}
+ if(defaultConstructor != null &&
(defaultConstructor.getModifiers() & Modifier.PUBLIC) == 0) {
--- End diff --
`if(defaultConstructor != null &&
Modifier.isPublic(defaultConstructor.getModifiers())` seems to be more readable
to me but your approach is fine too.
---
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 pull request: [FLINK-2437] Fix default constructor detection...
Github user ggevay commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-130050479 OK, I re-added the changes to remove the double "class". (Note, that the 3rd bullet in the opening comment refers to this doubling.) --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-129482084 Removing the double "class" is actually not bad. Was hard to get this from the diff alone. BTW: Simpler way may be to change the `aClass.toString()` call to `aClass.getCanonicalName()`. --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user ggevay commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-128819328 They print things like `Class class malom.GameState must have a default constructor to be used as a POJO.` If you think that the double "class" should be left the way it is, then I'm sorry for changing them. I have now removed the changes from the PR. I have also added a test. --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-128662225 Stephan is right. The changed messages just add noise to the pull request which makes it harder to review. --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-127046830 Why are you changing all messages in this pull request? They were not wrong before... --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user gyfora commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-126428191 Could you please add a test case that verifies the correct behaviour? --- 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 pull request: [FLINK-2437] Fix default constructor detection...
Github user ggevay commented on the pull request: https://github.com/apache/flink/pull/960#issuecomment-126398181 I removed the added "return null", I just realized that it is not needed. --- 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 pull request: [FLINK-2437] Fix default constructor detection...
GitHub user ggevay opened a pull request: https://github.com/apache/flink/pull/960 [FLINK-2437] Fix default constructor detection in TypeExtractor.analyzePojo - handle the case of a non-public default constructor - added a missing "return null" - removed some duplicating of the word "class" when printing class names, because class.toString also prepends it to the class name - fixed a typo in the documentation describing POJOs You can merge this pull request into a Git repository by running: $ git pull https://github.com/ggevay/flink analyzePojoFix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/960.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 #960 commit 7131c04ddc7ebce0b69bca1464c8312105ff9d92 Author: Gabor Gevay Date: 2015-07-30T15:38:43Z [FLINK-2437] Fix default constructor detection in TypeExtractor.analyzePojo - handle the case of a non-public default constructor - added a missing "return null" - removed some duplicating of the word "class" when printing class names, because class.toString also prepends it to the class name - fixed a typo in the documentation describing POJOs --- 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. ---
