Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/264#issuecomment-71333564
+1 for going for (1)
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/344#issuecomment-71548964
Looks good so far. I see that you removed the LRU code. Was that on purpose?
Leaving it in may be a good idea, because the soft references are cleared
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/337#issuecomment-71548405
Okay, if it is an auxiliary classpath then it should be fine.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/236#issuecomment-72432067
That is not a really good solution, IMHO.
Can we have a special tag that signifies that the deserialization is
delegated to Kryo?
How about this flow
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/236#issuecomment-72434125
Dang, I see your point. We need something more clever then...
Can we (for now) at least make sure that it fails early: Get an exception
when registering
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/265#issuecomment-72612157
Looks good so far. What about this PR is API breaking?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/354#issuecomment-72614564
So, this pull request can be closed?
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/333#issuecomment-72678344
I modified the merged version to exclude it from rat. I also tried
subsequent builds, it worked.
See here:
https://github.com/apache/flink/commit
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/359#issuecomment-73042551
Skipping the validation on raw types makes total sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/361#issuecomment-73041953
Will merge this...
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/134#issuecomment-73046443
I guess this one is subsumed by @rmetzger 's work on
https://issues.apache.org/jira/browse/FLINK-883
Can we close this PR?
---
If your project is set up
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/363#issuecomment-73040193
Does this occur during local execution, or collection execution? The
dependencies are not covered by the runtime dependencies?
---
If your project is set up
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/353#issuecomment-73042896
Looks good. For efficiency, we could change the factory such that it
returns the original in the first request, and a duplicate after that.
Good change
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/363#issuecomment-73014361
Looks good.
I have one suggestion concerning the Hadoop dependencies: The `flink-java`
project depends on the Hadoop API, for the `Writable` interface
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/360#issuecomment-73014844
Very nice, +1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/359#issuecomment-73013430
I do not quite get what this means now for the input validation.
Assume two classes, `A` and `B`, where `B` is a subclass of `A`.
```java
DataSetB data
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/365#issuecomment-73060980
Good one, thank you!
+1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/317#issuecomment-72618925
I think this is a good fix, overall. There is one issue I would really like
to fix, and that is the serializability of the `Instance` class. This class is
not meant
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/349#issuecomment-72631504
Looks good to me. I think the API breaking is minor, since it does not
affect the methods on `DataSet` or on any of the operators. The
TypeSerializerInputFormat
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/351#issuecomment-72629129
I tried to implement Henry's idea, but I noticed the error message is very
specific to the site where it is created, since it refers to an alternate
method
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/351#issuecomment-72623931
I'll merge this...
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/373#issuecomment-73477963
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/353#issuecomment-73478102
+1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/368#issuecomment-73478622
My bad, there is a ticket already. Can you squash the commits then and add
the ticket tag?
Otherwise, good to merge!
---
If your project is set up
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/374#issuecomment-73477910
Very nice work. I have one comment inline, otherwise +1 to go!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/374#discussion_r24315701
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/web/SetupInfoServlet.java
---
@@ -127,25 +134,42 @@ private void
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/368#issuecomment-73478074
Looks good. Since this is a behavior change, can you file a ticket for
this, Till?
---
If your project is set up for it, you can reply to this email and have your
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/296#issuecomment-69970228
How about setting the message to something like
Warning: You are running Flink with Java 6, which is not maintained any
more by Oracle or the OpenJDK
Github user StephanEwen commented on commit
7df6a3d7266b0f934b76722732176dbf5469bdb4:
https://github.com/apache/flink/commit/7df6a3d7266b0f934b76722732176dbf5469bdb4#commitcomment-9381927
In
flink-runtime/src/test/java/org/apache/flink/runtime/operators/sort
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/324#issuecomment-70953743
Looks good to me. It does not break the API, since it only moves source
code, so I do not have any objections. I would merge it into the master,
though, not into 0.8
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/301#issuecomment-70234744
BTW: How is that done in Hadoop? Can we follow a similar way, to make it
easier for users to understand this?
---
If your project is set up for it, you can reply
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/210#discussion_r23080920
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/accumulators/ListAccumulator.java
---
@@ -0,0 +1,97 @@
+/*
+ * Licensed
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/210#discussion_r23081185
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/AbstractIDTest.java ---
@@ -23,8 +23,8 @@
import static org.junit.Assert.fail
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/249#issuecomment-70257415
I think that this one can go into the 0.9 master now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/210#issuecomment-70258735
With the scheduler and intermediate data set enhancements coming up for 0.9
soon, this is now quite feasible to use. I suggest to merge it once the inline
comments
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/315#issuecomment-70358805
Great! I am looking forward very much to having that in the APIs
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/304#discussion_r22955775
--- Diff:
flink-core/src/test/java/org/apache/flink/api/common/typeutils/SerializerTestBase.java
---
@@ -99,6 +104,7 @@ public void testCopy
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/324#issuecomment-71103567
You have my +1
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/297#issuecomment-70413324
Thanks for taking a go at this. It is going into a good direction.
What needs to be adjusted is the way that the job manager gets accessed.
The current
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/236#issuecomment-70413969
Two questions relevant to the interplay with other code:
- How does the `registerType()` interact with the registration of custom
kryo serializers?
- Why
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/298#issuecomment-70421138
I addressed the comments and committed the changes to the 0.9 master. I
forgot to add the This closes #298 message, thought. Marton, can you close
this Pull Request
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/311#discussion_r23135998
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/operators/DualInputSemanticProperties.java
---
@@ -56,218 +56,135
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/291#issuecomment-70415920
Looks good to be merged, in my opinion!
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/292#issuecomment-70415876
A good piece of work!. I like that there is a much better test coverage for
the Yarn cluster now.
@rmetzger Can you comment on a few open questions
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/305#issuecomment-70416371
This would do it for now. However, the use of static variables to
communicate between components is an antipattern and one that has caused
problems before. I would
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/305#issuecomment-70424146
The registration at Kryo seems to be a bit incomplete. Adding a default
serializer does not to type/tag registration, which is very much desirable.
Also, it registers
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/304#discussion_r22956572
--- Diff:
flink-tests/src/test/scala/org/apache/flink/api/scala/runtime/KryoGenericTypeSerializerTest.scala
---
@@ -125,6 +133,7 @@ class
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/307#issuecomment-69967981
This change looks good to be merged...
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/404#issuecomment-74563335
Manually merged in b941cf2d091bb38b9a45e1f2412136acae2b0f3f
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/370#issuecomment-74565386
I think we should use the chained reducer from this pull request and drop
the chained combiner change.
---
If your project is set up for it, you can reply
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/404
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/392#issuecomment-74565272
At this point, we also have to worry about efficiency and performance.
This is also something that the user can easily mitigate in the UDF, at a
fraction
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/370#issuecomment-74236482
I think for the combiner, we cannot do this. The way it is done here easily
blows up the memory, by collecting so many records. Bear in mind that a record
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/385#discussion_r24653915
--- Diff:
flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala
---
@@ -530,54 +515,109 @@ Actor with ActorLogMessages
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/385#discussion_r24653930
--- Diff:
flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala
---
@@ -530,54 +515,109 @@ Actor with ActorLogMessages
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/385#discussion_r24657012
--- Diff:
flink-runtime/src/main/scala/org/apache/flink/runtime/jobmanager/JobManager.scala
---
@@ -530,54 +515,109 @@ Actor with ActorLogMessages
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/384#issuecomment-74240301
+1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/417#issuecomment-75019114
Good comments. I address them and merge this.
The tests that fail are not related to this change (there is an instability
in the TaskManager tests currently
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/404
[FLINK-1454] [job client] Improve error handling for failed connections
between JobClient and JobManager
- Decreases time until connection failure is discovered.
- Improves error
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/425#issuecomment-75094489
Looks good, thanks, I'll merge 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/419#issuecomment-75097617
Looks good, will merge this.
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/422#issuecomment-75102524
Looks good, will merge this as well...
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/410#issuecomment-74643785
I agree with Fabian that it is not a good default behavior to grab
everything that is possible.
It should be an explicit request by the user. For YARN single job
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/412
[FLINK-1561] [build system] Use a fresh fork for each integration test run
Helps the build stability on Travis for integration tests.
You can merge this pull request into a Git repository
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/338#issuecomment-71595710
I think in the current state, this makes sense.
I wrote the interface like it was, because it would enable implementations
that does not compute/provide all
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/311#issuecomment-71598228
+1 from my side
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/333#issuecomment-71598417
Okay, let's add an exclude for the linked target directory and update
`basedir` to `project.basedir`. Will that do?
---
If your project is set up for it, you can
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/328#issuecomment-71596308
Looks good to me
+1
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/210#issuecomment-71726730
@zentol You are right, for the time being, that this results in parts in
repeated execution. While not totally unavoidable in all cases, the code going
in soon about
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/343#issuecomment-71765382
+1
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/336#issuecomment-71416590
+1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/337#issuecomment-71412124
Does this mean that we now have multiple hadoop dependencies in the class
path? The ones that are part of the Flink lib directory, plus the ones
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/317#issuecomment-71423347
I am looking into this right now and testing it locally. Seems to work so
far. Will try to give a vote by tomorrow...
---
If your project is set up for it, you can
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/325#issuecomment-71157412
Looks 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/326#issuecomment-71157010
I took the pull request and filtered the branch to move the files into the
`flink-addons/flink-gelly` directory (that worked better for me than the
subtree merge
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/323#discussion_r23435527
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/typeutils/runtime/KryoSerializer.java
---
@@ -237,6 +244,25 @@ private void
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/315#issuecomment-71158420
+1 to merge from me as well. Go ahead, Timo...
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/202#issuecomment-71554357
Concerning the package / project structure: It is a bit humble and hidden
as some language binding for python.
I think this deserves to be more prominently
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/375#discussion_r24329907
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionJobVertex.java
---
@@ -260,15 +260,49 @@ public void
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/388#issuecomment-74080518
Good catch.
+1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/331#issuecomment-74068524
Looks like a good improvement to me.
+1 to merge
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/374#issuecomment-74068773
Looks good. There is a small conflict with #384 , but we can try and fix
this while merging.
+1
---
If your project is set up for it, you can reply
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/387#issuecomment-74068314
Looks good to me.
+1 to add
---
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
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/394#discussion_r24693247
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskManagerTest.java
---
@@ -486,7 +489,8 @@ protected void run
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/394#issuecomment-74311404
+1 Nice work and a critical fix. Prevents the root actors from failing on
user code problems.
---
If your project is set up for it, you can reply to this email
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/370#issuecomment-74247098
The synchronous combine driver stores them in the sort buffer in serialized
fashion, which is save.
---
If your project is set up for it, you can reply to this email
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/401
Fix standalone plan visualizer for 0.8 branch
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/StephanEwen/incubator-flink fix_plan_viz
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/477#issuecomment-83019386
Agree. The project re-structuring is not urgent. I would do it shortly
before the release.
---
If your project is set up for it, you can reply to this email and have
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/496#issuecomment-83105563
I'll do a pass over this and merge it today or tomorrow...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/499#issuecomment-83065881
Looks good.
+1 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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-83102692
With 236 files, that is hard to assess...
That is a point, in general, with humongous pull requests that touch 10
projects at the same time.
I would
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/486#issuecomment-81682932
Looks good to me.
---
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
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/442#issuecomment-81837584
Have a look at the build server logs. They still complain about unapproved
license headers.
---
If your project is set up for it, you can reply to this email
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/487
[FLINK-1671] Add different execution modes to APIs and optimizer.
This allows users to specify how programs should be executed:
- PIPELINED: Uses pipelining for shuffling, except where
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/471#issuecomment-82554365
Look good, nice test coverage and fits very well with the recent execution
mode changes.
Only downside: This pull request does contains many cases where only
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/492#issuecomment-82577264
We can do that, I am okay with this. So far, all renaming was strictly
internal.
The renaming of the maven project may affect some people who have added
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/479#issuecomment-82539185
Looks nice, +1 to add this.
Also, +1 to add the JIRA issue number to the commits and squash some basic
cleanup commits.
---
If your project is set up
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/488#issuecomment-82219329
I think this is very API breaking - in my opinion, we should keep the old
methods where it breaks the API, or at least keep them for one more version
(deprecated
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/442#issuecomment-82260288
Yes, this can be the problem. Can you add a licence header (with comments)
to this file?
---
If your project is set up for it, you can reply to this email and have
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/pull/482#issuecomment-82530221
I think this should go in. The simple the parameter style the better. And
without flags (just with arguments) reads easier, as far as I am concerned.
---
If your
1 - 100 of 4019 matches
Mail list logo
