[GitHub] flink pull request: Flink-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-86103055 Thank you for the pull request! --- 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-1780: Rename FlatCombineFunction to Grou...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/530 --- 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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on a diff in the pull request:
https://github.com/apache/flink/pull/530#discussion_r27135717
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/operators/RegularPactTask.java
---
@@ -1465,7 +1467,9 @@ public static void
cancelChainedTasks(List> tasks) {
for (int i = 0; i < tasks.size(); i++) {
try {
tasks.get(i).cancelTask();
- } catch (Throwable t) {}
+ } catch (Throwable t) {
+ // do nothing
+ }
}
--- End diff --
Same as above :) :)
---
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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on a diff in the pull request:
https://github.com/apache/flink/pull/530#discussion_r27135877
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
---
@@ -1425,11 +1425,9 @@ public static boolean isClassType(Type t) {
}
private static boolean sameTypeVars(Type t1, Type t2) {
- if (!(t1 instanceof TypeVariable) || !(t2 instanceof
TypeVariable)) {
- return false;
- }
- return ((TypeVariable)
t1).getName().equals(((TypeVariable)t2).getName())
- && ((TypeVariable)
t1).getGenericDeclaration().equals(((TypeVariable)t2).getGenericDeclaration());
+ return !(!(t1 instanceof TypeVariable) || !(t2 instanceof
TypeVariable)) &&
+ ((TypeVariable)
t1).getName().equals(((TypeVariable) t2).getName()) &&
+ ((TypeVariable)
t1).getGenericDeclaration().equals(((TypeVariable)
t2).getGenericDeclaration());
--- End diff --
Agreed. please feel free to revert this change.
---
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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on a diff in the pull request:
https://github.com/apache/flink/pull/530#discussion_r27135686
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/operators/RegularPactTask.java
---
@@ -525,7 +525,9 @@ protected void run() throws Exception {
try {
FunctionUtils.closeFunction(this.stub);
}
- catch (Throwable t) {}
+ catch (Throwable t) {
+ // do nothing
+ }
}
--- End diff --
The javadocs comment describe as to why we do a catch all and suppress all
exceptions/errors. :)
---
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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on a diff in the pull request: https://github.com/apache/flink/pull/530#discussion_r27135343 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/AllGroupCombineDriver.java --- @@ -35,20 +35,20 @@ * Like @org.apache.flink.runtime.operators.GroupCombineDriver but without grouping and sorting. May emit partially * reduced results. * -* @see org.apache.flink.api.common.functions.FlatCombineFunction +* @see GroupCombineFunction --- End diff -- You're right :) --- 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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-86091921 @mxm please go ahead --- 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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on a diff in the pull request: https://github.com/apache/flink/pull/530#discussion_r27133810 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/AllGroupCombineDriver.java --- @@ -35,20 +35,20 @@ * Like @org.apache.flink.runtime.operators.GroupCombineDriver but without grouping and sorting. May emit partially * reduced results. * -* @see org.apache.flink.api.common.functions.FlatCombineFunction +* @see GroupCombineFunction --- End diff -- Yes, since the path is in the imports, its redundant adding that here --- 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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-86090016 The documentation also needs to be adapted to use the new interface name. @smarthi If you don't mind I will merge your commit with my remarks addressed. --- 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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-85954079 +1 This rename makes a lot of sense. Looks good to me apart from the minor comments. --- 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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/530#discussion_r27104945
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/operators/RegularPactTask.java
---
@@ -1465,7 +1467,9 @@ public static void
cancelChainedTasks(List> tasks) {
for (int i = 0; i < tasks.size(); i++) {
try {
tasks.get(i).cancelTask();
- } catch (Throwable t) {}
+ } catch (Throwable t) {
+ // do nothing
+ }
}
--- End diff --
Same as above.
---
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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/530#discussion_r27104851
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/operators/RegularPactTask.java
---
@@ -525,7 +525,9 @@ protected void run() throws Exception {
try {
FunctionUtils.closeFunction(this.stub);
}
- catch (Throwable t) {}
+ catch (Throwable t) {
+ // do nothing
+ }
}
--- End diff --
A comment could be useful here to explain why we do a catch-all.
---
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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on a diff in the pull request: https://github.com/apache/flink/pull/530#discussion_r27104741 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/AllGroupCombineDriver.java --- @@ -35,20 +35,20 @@ * Like @org.apache.flink.runtime.operators.GroupCombineDriver but without grouping and sorting. May emit partially * reduced results. * -* @see org.apache.flink.api.common.functions.FlatCombineFunction +* @see GroupCombineFunction --- End diff -- Path to class missing here. --- 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-1780: Rename FlatCombineFunction to Grou...
Github user mxm commented on a diff in the pull request:
https://github.com/apache/flink/pull/530#discussion_r27104700
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
---
@@ -1425,11 +1425,9 @@ public static boolean isClassType(Type t) {
}
private static boolean sameTypeVars(Type t1, Type t2) {
- if (!(t1 instanceof TypeVariable) || !(t2 instanceof
TypeVariable)) {
- return false;
- }
- return ((TypeVariable)
t1).getName().equals(((TypeVariable)t2).getName())
- && ((TypeVariable)
t1).getGenericDeclaration().equals(((TypeVariable)t2).getGenericDeclaration());
+ return !(!(t1 instanceof TypeVariable) || !(t2 instanceof
TypeVariable)) &&
+ ((TypeVariable)
t1).getName().equals(((TypeVariable) t2).getName()) &&
+ ((TypeVariable)
t1).getGenericDeclaration().equals(((TypeVariable)
t2).getGenericDeclaration());
--- End diff --
Not sure if this change improves the readability of the code.
---
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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-85697927 @fhueske i think we shuld be LGTM now --- 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-1780: Rename FlatCombineFunction to Grou...
Github user smarthi commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-85688590 @fhueske yeah noticed that too, will update the PR --- 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-1780: Rename FlatCombineFunction to Grou...
Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/530#issuecomment-85688151 @smarthi Thanks for the PR! LGTM, except for a few lines where I spotted space indentions that do not comply with our code style. --- 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-1780: Rename FlatCombineFunction to Grou...
GitHub user smarthi opened a pull request: https://github.com/apache/flink/pull/530 Flink-1780: Rename FlatCombineFunction to GroupCombineFunction Fixes Flink-1780 You can merge this pull request into a Git repository by running: $ git pull https://github.com/smarthi/flink Flink-1780 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/530.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 #530 commit 47e721b73f34d0fc2c57d3ddc3942030f29997ae Author: Suneel Marthi Date: 2015-03-24T20:19:32Z Flink-1780: Rename FlatCombineFunction to GroupCombineFunction --- 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. ---
