Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3474
Yes, the `setEnvironment()` method will not be needed any more.
---
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 issue:
https://github.com/apache/flink/pull/3474
I would suggest to do the change differently. Right now, all the task logic
is pretty complex as it is, and this makes it even more complex.
My suggestion would be to
1. Change the
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3437
+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 and wishes so, or if the
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3440
+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 and wishes so, or if the
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3441
+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 and wishes so, or if the
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3439
+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 and wishes so, or if the
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3435
Good change, +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 and wishes so
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3356
I agree, the CLI should accept dynamic options that are logically added to
the configuration. But we still do not need the extra dynamic properties
mechanism when setting up the TaskManager and
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3424
Looks good, merging 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 this feature
enabled and
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3420#discussion_r103481458
--- Diff:
flink-connectors/flink-connector-kafka-0.8/src/main/java/org/apache/flink/streaming/connectors/kafka/internals/KillerWatchDog.java
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3420#discussion_r103481121
--- Diff:
flink-connectors/flink-connector-kafka-0.8/src/main/java/org/apache/flink/streaming/connectors/kafka/internals/ClosableBlockingQueue.java
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3411
Thanks for the review, @uce - addressing the comments and merging 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
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3303
I have merged this to my local repository.
There were some issues left, partly in the commented out code.
In particular `checkNotNull(variable !=null) does not work, because
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3385
One test case seemed to be failing in this PR:
I have merged the PR to my local repository, fixed the test, and added some
fixes/cleanups on top.
Will merge back to Flink master tomorrow
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3424
Thanks, good and critical fix!
Looking at 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
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3391
@barcahead Thansk for contributing this.
I can try and get to this later this week. Big pull request backlog right
now ;-)
---
If your project is set up for it, you can reply to this email
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3385
Thanks!
I think I can take this over 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
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3340#discussion_r102993824
--- Diff:
flink-core/src/main/java/org/apache/flink/configuration/ConfigConstants.java ---
@@ -110,7 +110,13 @@
public static final String
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3340#discussion_r102995512
--- Diff:
flink-runtime/src/test/java/org/apache/flink/runtime/executiongraph/ExecutionVertexCancelTest.java
---
@@ -440,6 +440,72 @@ public void
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3340
There is a lot of good code in this PR.
What I would suggest to make different is to NOT make `Execution`,
`IntermediateResult` partition, etc mutable. There is a big benefit to having
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3385#discussion_r102993092
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/highavailability/ZookeeperRegistry.java
---
@@ -55,7 +59,7 @@ public void setJobRunning
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3385
With the problem observed above, I think we should change the approach a
bit:
- The registry should have an enum that it returns:
`getJobSchedulingStatus` or so, which can be `PENDING
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3385
One issue I think can happen in practice is that the checks "isRunning" and
"isFinished" are not atomic. Imagine this scenario:
- job is running
- JobManage
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3385
I would like to merge this and make a few edits on top...
---
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
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3385
PR looks like a good start, but I think we need to add a few things on top:
- The file-based registry cannot distinguish between "job created but not
running" and "job
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3303
One thought that @tillrohrmann and me had: It is probably okay to comment
out or remove the **setters** and keep the **getters**. That should help in
keeping the internal code.
---
If your
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102942657
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/operators/Operator.java ---
@@ -45,6 +45,10 @@
private int
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102946206
--- Diff:
flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/DataStream.scala
---
@@ -144,6 +145,43 @@ class DataStream[T
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102946762
--- Diff:
flink-scala/src/main/scala/org/apache/flink/api/scala/DataSet.scala ---
@@ -178,6 +178,60 @@ class DataSet[T: ClassTag](set: JavaDataSet[T
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102946397
--- Diff:
flink-scala/src/main/scala/org/apache/flink/api/scala/DataSet.scala ---
@@ -178,6 +178,60 @@ class DataSet[T: ClassTag](set: JavaDataSet[T
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102945697
--- Diff:
flink-streaming-scala/src/main/scala/org/apache/flink/streaming/api/scala/DataStream.scala
---
@@ -144,6 +145,43 @@ class DataStream[T
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102945079
--- Diff:
flink-streaming-java/src/main/java/org/apache/flink/streaming/api/transformations/StreamTransformation.java
---
@@ -126,6 +127,18 @@ public
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3303#discussion_r102942856
--- Diff:
flink-java/src/main/java/org/apache/flink/api/java/operators/DataSink.java ---
@@ -278,4 +283,60 @@ public int getParallelism
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3303
A more general question on the resource matching: If I understand it
correctly, then the resource manager will try to get the "max" resources for an
operator, but potentially go down t
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3303
The code here looks very good, with a few minor comments.
The main problem is as you mentioned: We are adding something to the API
that is not yet supported by the runtime. We have done
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3402
Fix looks good.
Is this tested implicitly by some other test already?
---
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 issue:
https://github.com/apache/flink/pull/3403
Looks good, +1 to 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 this feature
enabled
GitHub user StephanEwen opened a pull request:
https://github.com/apache/flink/pull/3411
[FLINK-5897] & [FLINK-5822] First step towards Generic State Backends and
Global State Cleanup Hooks
**This is the first part of a larger parent issue: Self-contained
external
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/commit/30c9e2b683bf7f4776ffc23b6a860946a4429ae5#commitcomment-21018833
Great to have this in!
I would suggest to mention in the JavaDocs of `MapState.size()` that this
can be a potentially
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3368
Thanks for the comments. Will address the issues and remove the "no
message, no cause" constructors. We should not encourage exceptions without
information.
---
If your project is
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3373
If you can remove the comment, that would be great. Otherwise we can 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
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3373#discussion_r102753321
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java ---
@@ -109,6 +109,8 @@
private boolean forceKryo
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3374
I think having it only in the configuration is probably fine. I think we do
not need both paths here.
Illegal values are probably checked when creating the recovery factory.
It would
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3392
Looks correct, +1 to 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 this feature
enabled
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3310
Looks very good to me!
+1 to 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 this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3400
Good catch, thanks! Merging 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 this feature
Github user StephanEwen commented on the pull request:
https://github.com/apache/flink/commit/646490c4e93eca315e4bf41704f149390f8639cc#commitcomment-21003603
Should we make `flushOnCheckpoint` true by default?
---
If your project is set up for it, you can reply to this email and
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3384
Looks good, +1 to merge
Optional comment: I have seen that developers get confused when working
with the code whether a `long` refers to a "millisecond" timestamp or to a
&
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3346#discussion_r102472651
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/checkpoint/CheckpointCoordinator.java
---
@@ -296,15 +298,42 @@ public boolean
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3346
This is a prerequisite for https://issues.apache.org/jira/browse/FLINK-5820
Reviewing this now...
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3345
Very good change! Looked through it, nothing to complain about ;-)
Merging this...
---
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 issue:
https://github.com/apache/flink/pull/3360
Looking at this from another angle: If any Runnable that is scheduled ever
lets an exception bubble out, can we still assume that the JobManager is in a
sane state? Or should be actually make
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3368
Any reservations against merging this after addressing the comments 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
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3368#discussion_r102431100
--- Diff:
flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java
---
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3368#discussion_r102430926
--- Diff:
flink-core/src/main/java/org/apache/flink/util/DynamicCodeLoadingException.java
---
@@ -0,0 +1,52 @@
+/*
+ * Licensed to the Apache
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3373#discussion_r102429969
--- Diff:
flink-core/src/test/java/org/apache/flink/api/common/ExecutionConfigTest.java
---
@@ -64,4 +68,15 @@ public void
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3380
I think it is a good idea to avoid exception wrapping where ever possible,
so +1 to that.
I was wondering if we can improve exception handling even further for the
state abstraction
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3384#discussion_r102428106
--- Diff:
flink-clients/src/test/java/org/apache/flink/client/program/ClientConnectionTest.java
---
@@ -115,13 +115,13 @@ public void run
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3190
Hi! `mvn test` works from the command line.
IntelliJ right-click on a test and run does often not work, it only works
if a console build was done before and the `.../target/tmp
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3374
Hi @tony810430 thank you for the pull request!
The code looks good.
My feeling is, though, that the number of checkpoints to retain is
something that we want rather in the
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/3328
---
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 issue:
https://github.com/apache/flink/pull/3328
Yes, it was accidentally merged together with another PR.
Since the changes were mostly comments and annotations, it should not be a
problem.
Closing...
---
If your project is set
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3190
@shijinkui This still has the problem that tests don't run any more from
within the IDE. That is a big problem for developers.
As an alternative, we can always fix the tests that d
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3368
It is arguable whether exceptions should ever have a constructor without a
message, I simply did that here for convenience. I have no strong feelings
about removing the zero argument
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3368
@zentol There are many places in the runtime that declare `throws
Exception`, for example virtually all of the state handling code. This always
came from the desire to throw `IOException` plus
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2903
I think this looks good, thanks!
Merging 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 opened a pull request:
https://github.com/apache/flink/pull/3368
[FLINK-5854] [core] Add base Flink Exception classes
This pull request adds two exception base classes: `FlinkException` and
`FlinkRuntimeException`.
They are useful in improving the way
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3190
@shijinkui The FileCacheDeleteValidationTest should still be fixed now.
When I tested it, I combined the changed from FLINK-5817 and this pull request.
---
If your project is set up for it, you
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3274
Looks good, merging 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 this feature
enabled and
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3288
Looks good, thank you!
Merging 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 this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3360
I would suggest that we adopt the following pattern for all the places like
the one in this pull request where we catch Throwables:
```java
try {
...
} catch (Throwable t
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3190
Sorry, I have to actually step back on this one.
I merged it into a feature branch and played around a bit with this, and it
turns out it is not possible any more to execute tests from
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3335
@WangTaoTheTonic Am I right in assuming that your scenario assumes that
multiple different users submit Flink jobs and these jobs cannot be "prepared"
by a script that sets up a
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3295#discussion_r101829659
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionGraph.java
---
@@ -754,6 +759,139 @@ public void
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3295#discussion_r101829019
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/concurrent/FutureUtils.java
---
@@ -88,4 +100,104 @@ public RetryException(Throwable
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3295#discussion_r101827724
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/concurrent/FutureUtils.java
---
@@ -88,4 +100,104 @@ public RetryException(Throwable
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3334
Thank you for opening this pull request.
I'll try to review it in the coming days...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitH
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3323
Good cleanup, thanks a lot!
Merging 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 issue:
https://github.com/apache/flink/pull/3322
The change looks good, thank you!
Merging 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
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3309
Good addition, thanks!
Merging 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 this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3292
Change looks good, thank you!
Merging 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 issue:
https://github.com/apache/flink/pull/3249
Looks good.
+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 this feature
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3243
Subsumed by another 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
Github user StephanEwen closed the pull request at:
https://github.com/apache/flink/pull/3243
---
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 issue:
https://github.com/apache/flink/pull/3231
@billliuatuber Since the dependency management section in the root
`pom.xml` defines the Scala version for all sub-modules, I think this change is
not needed.
If you agree, could you close
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3223
This change looks good, thank you!
Merging 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
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3211
Change looks good, thanks!
Merging 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 issue:
https://github.com/apache/flink/pull/3204
Does anyone want to take a stab at addressing this the
https://issues.apache.org/jira/browse/FLINK-4326 way? I think no one is active
on that issue right now...
---
If your project is set up
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3190
All right, I am convinced now that this is a helpful change.
Merging this...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3341
Good change, thank you. merging 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 this feature
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3138
I think that is a good fix, thank you!
Merging 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 issue:
https://github.com/apache/flink/pull/3089
Thank you for fixing 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 this feature
enabled and
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3089
Okay, I finally found the time to double check this.
The changes are good, merging this...
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3085
I would personally prefer to not make that change at this point.
Interpreting HA parameters in non-HA mode might come across as confusing to
users.
Also, the new way of instantiating
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3084
Good change, thanks!
Merging 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 this
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3349
Awesome to see that Flink is that easy to install in DC/OS...
Merging this...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/3342
Merging 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 this feature
enabled and wishes so
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3295#discussion_r101754578
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/concurrent/FutureUtils.java
---
@@ -88,4 +100,104 @@ public RetryException(Throwable
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3295#discussion_r101753992
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionGraph.java
---
@@ -292,7 +305,8 @@ public ExecutionGraph
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/3295#discussion_r101753888
--- Diff:
flink-runtime/src/main/java/org/apache/flink/runtime/executiongraph/ExecutionGraph.java
---
@@ -754,6 +759,139 @@ public void
1101 - 1200 of 4278 matches
Mail list logo
