You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by smarthi <gi...@git.apache.org> on 2014/11/24 00:46:02 UTC
[GitHub] incubator-flink pull request: FLINK-1276
GitHub user smarthi opened a pull request:
https://github.com/apache/incubator-flink/pull/227
FLINK-1276
Fixes FLINK-1276
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/smarthi/incubator-flink suneel
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-flink/pull/227.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 #227
----
commit 1753024d3bd002ec65f4ca449a0f6fd48553a6d8
Author: Suneel Marthi <su...@gmail.com>
Date: 2014-11-23T23:43:37Z
Fixes FLINK-1276
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20788933
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/hash/CompactingHashTable.java ---
@@ -718,8 +719,7 @@ private void createPartitions(int numPartitions) {
}
private void clearPartitions() {
- for (int i = 0; i < this.partitions.size(); i++) {
- InMemoryPartition<T> p = this.partitions.get(i);
+ for (InMemoryPartition<T> p : this.partitions) {
--- End diff --
The for-each syntax creates an iterator, while the for loop with indexes does not.
I think this is not critical here, but in general, I have used the indexed access in some parts on purpose to avoid iterator creation and circumvent the danger of a concurrent modification exception.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by smarthi <gi...@git.apache.org>.
Github user smarthi closed the pull request at:
https://github.com/apache/incubator-flink/pull/227
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20789890
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/hash/HashPartition.java ---
@@ -44,8 +46,8 @@
/**
*
- * @param BT The type of the build side records.
- * @param PT The type of the probe side records.
+ * BT - The type of the build side records.
+ * PT - The type of the probe side records.
--- End diff --
Should this be `@param <PT>` ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20788734
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/CoLocationConstraint.java ---
@@ -49,10 +49,7 @@ public Instance getLocation() {
}
public void setSharedSlot(SharedSlot sharedSlot) {
- if (this.sharedSlot == sharedSlot) {
--- End diff --
Is this correct? Does the second condition subsume the first?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the pull request:
https://github.com/apache/incubator-flink/pull/227#issuecomment-64196769
@smarthi I have some inline comments / questions at some points. What is your opinion on those issues?
Most of the changes are good fixes.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20788430
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/CollectionEnvironment.java ---
@@ -24,13 +24,11 @@
public class CollectionEnvironment extends ExecutionEnvironment {
- private boolean mutableObjectSafeMode = true;
--- End diff --
We actually want to make the execution mode configurable with a setter method, so this would actually be god to stay.
We are thinking about how to do the configuration:
- A set of setters
- A configuration/parameter object
I am leaning towards the second option, because we will have many more things to configure (closure cleaning, custom serializers, file system modes, mutable object modes)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by hsaputra <gi...@git.apache.org>.
Github user hsaputra commented on the pull request:
https://github.com/apache/incubator-flink/pull/227#issuecomment-64285034
@smarthi, this PR contains multiple fixes for different issues and improvement on code styles.
Could you reduce the fix to do what the original PR want to do or the other way is to expand the description of the PR to explain things included in 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20788520
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/typeutils/ValueTypeInfo.java ---
@@ -131,7 +127,7 @@ public String toString() {
// --------------------------------------------------------------------------------------------
- static final <X extends Value> TypeInformation<X> getValueTypeInfo(Class<X> typeClass) {
--- End diff --
The `final` modifiers in are technically redundant on such methods (as they are on private methods). To me, they still sort of express an intent that the logic is the function is a hard assumption that should not be easily swapped for something else. This, the modifier is a soft documentation token...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20790487
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/util/EnvironmentInformation.java ---
@@ -98,7 +98,7 @@ public static String getUserRunning() {
return UserGroupInformation.getCurrentUser().getShortUserName();
}
catch (Throwable t) {
- if (LOG.isDebugEnabled() && !(t instanceof ClassNotFoundException)) {
--- End diff --
I actually added this check on purpose to avoid a confusing error message in local execution (out of the IDE), where Hadoop is frequently not in the classpath.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-flink pull request: FLINK-1276
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/incubator-flink/pull/227#discussion_r20790380
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/operators/hash/HashPartition.java ---
@@ -297,9 +299,7 @@ public int spillPartition(List<MemorySegment> target, IOManager ioAccess, FileIO
}
// return the memory from the overflow segments
- for (int i = 0; i < this.numOverflowSegments; i++) {
- target.add(this.overflowSegments[i]);
- }
+ target.addAll(Arrays.asList(this.overflowSegments).subList(0, this.numOverflowSegments));
--- End diff --
The statement is again more efficient than the one below (which is somewhat better readable).
I think it is good in this case, as the spillPartition() method is not part of a an inner loop. Inside inner loops, I try to be very careful with such operations, to keep object allocations and indirections to a minimum.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---