[GitHub] flink issue #3749: Typo sick -> sink

2017-04-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3749 Good fix, funny typo ;-) 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] flink issue #3747: [FLINK-5623] [runtime] Fix TempBarrier dam has been close...

2017-04-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3747 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] flink issue #3648: [FLINK-6217] ContaineredTaskManagerParameters sets off-he...

2017-04-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3648 Good fix, thanks @haohui I was wondering - I am a trying to advocate fewer dependencies in Flink (there is always the problem of shading and conflicts) so if there is a way to do

[GitHub] flink issue #3704: [FLINK-5756] Replace RocksDB dependency with FRocksDB

2017-04-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3704 @hsaputra There is a plan to do that and we are in touch with the RocksDB folks. The latest RocksDB master does not work for Flink though, currently, so we needed a custom backport to an earlier

[GitHub] flink issue #3747: [FLINK-5623] [runtime] Fix TempBarrier dam has been close...

2017-04-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3747 +1 to this fix --- 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] flink issue #3741: [FLINK-6177] Add support for "Distributed Cache" in strea...

2017-04-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3741 Looks quite good now. If I can ask you for one more followup: To have faster tests, it would be good to add the streaming distributed cache test and the batch distributed cache test

[GitHub] flink issue #3701: [FLINK-6280] [scripts] Allow logging with Java flags

2017-04-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3701 Okay, got it. The docs and examples did help a lot. This looks good to me, +1 to merge --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] flink issue #3598: [FLINK-6103] LocalFileSystem rename() uses File.renameTo(...

2017-04-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3598 Thanks, I'll take it from 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

[GitHub] flink issue #3598: [FLINK-6103] LocalFileSystem rename() uses File.renameTo(...

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3598 I would do the following: - log nothing - Catch the errors that are regular "move failed" exceptions and return false. - `FileNotFoun

[GitHub] flink issue #3127: [FLINK-5481] Simplify Row creation

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3127 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] flink issue #3738: [FLINK-6311] [Kinesis Connector] NPE in FlinkKinesisConsu...

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3738 The `shardConsumersExecutor` variable is final. No need to make it `volatile`. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] flink issue #3722: [FLINK-5646] Document JAR upload with the REST API

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3722 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 this feature

[GitHub] flink issue #3709: [FLINK-6295]use LoadingCache instead of WeakHashMap to lo...

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3709 @WangTaoTheTonic Yes, that is correct. @zentol's suggestion should work. On access, if the `JobStatus` is suspended, remove the entry from the `WeakHashMap`. --- If your project is set

[GitHub] flink issue #3703: [FLINK-5005] WIP: publish scala 2.12 artifacts

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3703 We cannot add any other dependencies to the pom files. Adding "akka" back will create a conflict with the "flakka" files. What we can do is wither of the f

[GitHub] flink issue #3703: [FLINK-5005] WIP: publish scala 2.12 artifacts

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3703 One thing you can try and do is to run `TypeExtractionUtils.checkAndExtractLambda` to see if it is a generated serializable Lambda. In the case of a Lambda, you could switch to a different

[GitHub] flink issue #3701: [FLINK-6280] [scripts] Allow logging with Java flags

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3701 Can you explain a bit more what this change does exactly? Does it make sure that if you put bash commands into the java opts to compute flags, it evaluates those? Can you elaborate why

[GitHub] flink issue #3727: [FLINK-6312]update curator version to 2.12.0 to avoid pot...

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3727 Good idea to upgrade Curator. Unfortunately, it seems some behavior in Curator has changed. The change causes many tests to hang/fail: https://travis-ci.org/apache/flink/builds

[GitHub] flink issue #3736: [Flink-6013][metrics] Add Datadog HTTP metrics reporter

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3736 A few more comments on this: - Guava is so conflict heavy that should avoid using it in the framework wherever possible. Adding a multiple MBs dependency to write `Lists.newArrayList

[GitHub] flink issue #3738: [FLINK-6311] [Kinesis Connector] NPE in FlinkKinesisConsu...

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3738 Looks good. To make this really safe, we should actually make the `mainThread` variable `volatile`. --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] flink issue #3741: [FLINK-6177] Add support for "Distributed Cache" in strea...

2017-04-19 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3741 Thanks for contributing this, the added functionality looks good. I would prefer to add this change without changing the dependencies and test base classes. You could for example change

[GitHub] flink issue #3704: [FLINK-5756] Replace RocksDB dependency with FRocksDB

2017-04-09 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3704 @SyinChwunLeo We will try and contribute the patch to RocksDB and will also soon try and move to a newer RocksDB version, as soon as its Java API works again for the required functions

[GitHub] flink issue #3525: [FLINK-6020]add a random integer suffix to blob key to av...

2017-04-05 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3525 I am currently travelling and then attending Flink Forward. Will come back to this after that. Quick feedback: - I am still thinking that the random suffix breaks the original idea

[GitHub] flink issue #3563: [FLINK-2814] [optimizer] DualInputPlanNode cannot be cast...

2017-04-03 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3563 I think cherry-picking this into the `release-1.2` branch would be nice --- 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] flink issue #2753: [FLINK-4840] [metrics] Measure latency of record processi...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2753 A gentle ping about how to proceed here... Are you interested in pursuing another implementation approach, or should we close this as "fix later"? --- If your project

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108671962 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108671268 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108670950 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108670845 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108670700 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108671445 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108670677 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink pull request #3524: [FLINK-6014][checkpoint] Allow the registration of...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3524#discussion_r108670567 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/state/SharedStateRegistry.java --- @@ -0,0 +1,192 @@ +/* + * Licensed

[GitHub] flink issue #3598: [FLINK-6103] LocalFileSystem rename() uses File.renameTo(...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3598 I think we don't need the logging in all of the cases. We typically avoid logging in "utility functions", which are missing the context if the exception is in fact a problem worth

[GitHub] flink issue #3563: [FLINK-2814] [optimizer] DualInputPlanNode cannot be cast...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3563 @greghogan I think you can go ahead and 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

[GitHub] flink issue #3632: [FLINK-6176] [scripts] Add JARs to CLASSPATH deterministi...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3632 Looks like a good change. Do we want the same deterministic class path order also when launching Yarn and Mesos containers? /cc @rmetzger @EronWright --- If your project is set up

[GitHub] flink issue #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3633 Thanks, I will try and look at this over the next week. It is quite a big change, so I might need a bit of time to review it properly... --- If your project is set up for it, you can reply

[GitHub] flink issue #3642: SignalWindow

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3642 I think this will not work. Windows are keys in hash tables. Mutating a key while it is already in the hash table makes it effectively impossible to find or delete any more. --- If your

[GitHub] flink issue #3639: Update TimeWindow.java

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3639 Windows are immutable and must be, as they are keys in hash tables. I am also not sure how this code can possibly compile, as it tries to update a final field. For changes

[GitHub] flink issue #3640: [FLINK-6213] [yarn] terminate resource manager itself whe...

2017-03-29 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3640 Good idea to add the poison pill. But does it actually work when the poison pill is decorated? --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] flink issue #3633: [FLINK-5982] [runtime] Refactor AbstractInvokable and Sta...

2017-03-28 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3633 Thanks for this refactoring. I had only a quick look so far, but one thought was that this patch could probably be simplified by making `triggerCheckpoint()` (and restore / notifyComplete

[GitHub] flink pull request #3610: [FLINK-6183]/[FLINK-6184] Prevent some NPE and unc...

2017-03-28 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3610#discussion_r108478998 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/metrics/groups/TaskManagerJobMetricGroup.java --- @@ -80,8 +80,17 @@ public

[GitHub] flink issue #3627: Release 0.4 alpha.0

2017-03-28 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3627 Can you close this PR? Looks like this is some confusion... --- 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] flink issue #3600: [FLINK-6117]Make setting of 'zookeeper.sasl.disable' work...

2017-03-28 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3600 I am a bit confused here, this seems to make things more complicated. Before, if you wanted to use ZK and Kerberos, you only added `zookeeper` to `security.kerberos.login.contexts`. Now

[GitHub] flink issue #2252: [FLINK-3466] [runtime] Cancel state handled on state rest...

2017-03-27 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/2252 I think it is yes. We worked around it in the meantime... --- 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] flink pull request #1668: [FLINK-3257] Add Exactly-Once Processing Guarantee...

2017-03-25 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/1668#discussion_r108034229 --- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/runtime/tasks/StreamIterationTail.java --- @@ -64,6 +70,22 @@ public void init

[GitHub] flink pull request #3602: [FLINK-5715] Asynchronous snapshots for heap keyed...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3602#discussion_r107931282 --- Diff: flink-tests/src/test/java/org/apache/flink/test/checkpointing/AbstractEventTimeWindowCheckpointingITCase.java --- @@ -115,6 +117,14 @@ public

[GitHub] flink issue #3600: [FLINK-6117]Make setting of 'zookeeper.sasl.disable' work...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3600 @EronWright @vijikarthi If you find the time, what do you think about this patch? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well

[GitHub] flink issue #3600: [FLINK-6117]Make setting of 'zookeeper.sasl.disable' work...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3600 Yes, CI seems to be unstable. Can you try to push again to the branch (maybe change the commit message, force push then) to retrigger the CI? --- If your project is set up for it, you

[GitHub] flink issue #3599: [FLINK-6174][HA]introduce a new election service to make ...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3599 I would suggest to fix this the following way: - There is an upcoming patch that makes the Flink codebase use the `HighAvailabilityServices` properly in all places. - We

[GitHub] flink pull request #3598: [FLINK-6103] LocalFileSystem rename() uses File.re...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3598#discussion_r107925602 --- Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java --- @@ -262,8 +265,13 @@ public FSDataOutputStream create

[GitHub] flink pull request #3605: [FLINK-6181][Start scripts] Fix regex in start scr...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3605#discussion_r107923309 --- Diff: tools/travis_mvn_watchdog.sh --- @@ -164,7 +164,7 @@ watchdog () { # Check the final fat jar for illegal artifacts

[GitHub] flink pull request #3598: [FLINK-6103] LocalFileSystem rename() uses File.re...

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3598#discussion_r107863788 --- Diff: flink-core/src/main/java/org/apache/flink/core/fs/local/LocalFileSystem.java --- @@ -262,8 +265,13 @@ public FSDataOutputStream create

[GitHub] flink issue #3583: [FLINK-6043] [web] Display exception timestamp

2017-03-24 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3583 Can we remove the `errorTimestamp` from the `notifyStateTransition`? I think it obfuscates the meaning a bit... --- If your project is set up for it, you can reply to this email and have your

[GitHub] flink issue #3599: [FLINK-6174][HA]introduce a new election service to make ...

2017-03-23 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3599 -1 sorry. This needs to go to the drawing board (FLIP or detailed JIRA discussion) before we consider a change that is impacting the guarantees and failure mode so heavily

[GitHub] flink issue #3515: [FLINK-5890] [gelly] GatherSumApply broken when object re...

2017-03-23 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3515 It is a +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] flink issue #3592: [FLINK-5977] Rename MAX_ATTEMPTS_HISTORY_SIZE key

2017-03-22 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3592 Thanks for the pull request. This has actually already been fixed in https://github.com/apache/flink/commit/81a143f6b42cf39d56a36222d14b5db0cc54addb The commit also retains the old

[GitHub] flink issue #3588: [FLINK-6144] [config] Port JobManager configuration optio...

2017-03-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3588 Good change, +1 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] flink issue #3506: [FLINK-6000] Fix starting HA cluster with start-cluster.s...

2017-03-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3506 Good change, merging this for `1.2.1` and `1.3`. --- 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] flink issue #3540: [FLINK-6056] [build]apache-rat exclude flink directory in...

2017-03-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3540 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] flink issue #3556: [FLINK-6084][Cassandra] Promote transitive dependencies

2017-03-21 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3556 Looks good to me, +1 to merge for both `master` and `release-1.2` --- 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] flink issue #3567: [FLINK-6107] Add custom checkstyle for flink-streaming-ja...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3567 It would be awesome if we could make IntelliJ pick up the style config automatically. By committing some files under ".Idea", we might be able to do that... On Mar 20,

[GitHub] flink issue #3539: Flip1: fine gained recovery

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3539 @shuai-xu Had a look at the changes. In general, I like the decoupling of failover handling from the execution graph. Some suggestions: - How about we rename

[GitHub] flink issue #3521: [FLINK-6027][checkpoint] Ignore the exception thrown by t...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3521 Looks good, thanks. Merging... --- 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] flink issue #3567: [FLINK-6107] Add custom checkstyle for flink-streaming-ja...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3567 Can you describe what rules this style actually enforces? --- 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] flink issue #3540: [FLINK-6056] [build]apache-rat exclude flink directory in...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3540 @shijinkui Got it, thanks. +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

[GitHub] flink issue #3563: [FLINK-2814] [optimizer] DualInputPlanNode cannot be cast...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3563 Nice, looks like a compact fix. With the explanation about test coverage, +1 from my side --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] flink issue #3524: [FLINK-6014][checkpoint] Allow the registration of state ...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3524 Can you give me some background why you want to make also `PendingCheckpoint` register its state immediately (and not only upon completion)? I see no problem with that, just want to double

[GitHub] flink issue #3535: [FLINK-5713] Protect against NPE in WindowOperator window...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3535 I can not really assess what this change is doing in detail, meaning how it affects typical work loads. Would need a bit more context. The change looks small an innocent and seems

[GitHub] flink issue #3524: [FLINK-6014][checkpoint] Allow the registration of state ...

2017-03-20 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3524 @shixiaogang Thanks for the fast response! Can y In the initial design document, you suggest that shared state is only s I think what we need is a subclass of `StateHandle

[GitHub] flink issue #3525: [FLINK-6020]add a random integer suffix to blob key to av...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3525 I think we should then fix this in the blob server. The problem that only one should succeed upon collision should be fixable by using `Files.move()` with `ATOMIC_MOVE`. Only when

[GitHub] flink pull request #3127: [FLINK-5481] Simplify Row creation

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3127#discussion_r106717641 --- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/Types.scala --- @@ -17,29 +17,51 @@ */ package

[GitHub] flink issue #3524: [FLINK-6014][checkpoint] Allow the registration of state ...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3524 One more question: Can the StateRegistry not directly drop states that have no reference any more when states are unregistered? Is there a special reason for first collecting these states

[GitHub] flink issue #3524: [FLINK-6014][checkpoint] Allow the registration of state ...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3524 Thanks for opening this pull request. Adding a `CompositeStateHandle` and a `StateRegistry` is a good idea. Some thoughts: - What do you think about making the `StateRegistry

[GitHub] flink issue #3537: [FLINK-6050] [robustness] Register exception handler on t...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3537 I think this is good, +1 Do we have a test that validates that completing a `Future` exceptionally also completes all result Futures of `thenApply` (or `thenApplyAsync`) functions

[GitHub] flink issue #3521: [FLINK-6027][checkpoint] Ignore the exception thrown by t...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3521 Actually, do you think you could add a test for this? Would be good to guard that for the future... --- If your project is set up for it, you can reply to this email and have your reply appear

[GitHub] flink issue #3526: [FLINK-5999] [resMgnr] Move JobLeaderIdService shut down ...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3526 Looks good 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 as well. If your project does not have this feature enabled

[GitHub] flink pull request #3512: [FLINK-6008] collection of BlobServer improvements

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3512#discussion_r106674262 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobCache.java --- @@ -180,91 +180,159 @@ public URL getURL(final BlobKey

[GitHub] flink pull request #3512: [FLINK-6008] collection of BlobServer improvements

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3512#discussion_r106677990 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java --- @@ -400,6 +418,47 @@ public void delete(BlobKey key) throws

[GitHub] flink pull request #3512: [FLINK-6008] collection of BlobServer improvements

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3512#discussion_r106674837 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobCache.java --- @@ -180,91 +180,159 @@ public URL getURL(final BlobKey

[GitHub] flink pull request #3512: [FLINK-6008] collection of BlobServer improvements

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3512#discussion_r106675058 --- Diff: docs/setup/config.md --- @@ -494,13 +494,13 @@ Previously this key was named `recovery.mode` and the default value was `standal

[GitHub] flink pull request #3512: [FLINK-6008] collection of BlobServer improvements

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3512#discussion_r106680476 --- Diff: flink-runtime/src/main/scala/org/apache/flink/runtime/taskmanager/TaskManager.scala --- @@ -1305,6 +1305,9 @@ class TaskManager

[GitHub] flink pull request #3512: [FLINK-6008] collection of BlobServer improvements

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3512#discussion_r106677799 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/blob/BlobServer.java --- @@ -400,6 +418,47 @@ public void delete(BlobKey key) throws

[GitHub] flink issue #3521: [FLINK-6027][checkpoint] Ignore the exception thrown by t...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3521 +1, 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

[GitHub] flink issue #3525: [FLINK-6020]add a random integer suffix to blob key to av...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3525 I don't quite understand the issue. Currently, the name should exactly match the hash to make sure that each library is stored only once. Adding a random suffix exactly destroys that behavior

[GitHub] flink pull request #3127: [FLINK-5481] Simplify Row creation

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3127#discussion_r106664092 --- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/Types.scala --- @@ -17,29 +17,51 @@ */ package

[GitHub] flink issue #3543: [FLINK-5985] [Backport for 1.2] Report no task states for...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3543 Looks good. +1 for 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] flink pull request #3127: [FLINK-5481] Simplify Row creation

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3127#discussion_r106644720 --- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/Types.scala --- @@ -17,29 +17,51 @@ */ package

[GitHub] flink issue #3127: [FLINK-5481] Simplify Row creation

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3127 Found one more small concern (inline comment) --- 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] flink issue #3127: [FLINK-5481] Simplify Row creation

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3127 Looks good to me, +1 @twalthr @fhueske Any concerns about 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

[GitHub] flink issue #3392: [FLINK-5883] Re-adding the Exception-thrown code for List...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3392 +1 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

[GitHub] flink issue #3503: [FLINK-5995][checkpoints] fix Get a Exception when creati...

2017-03-17 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3503 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

[GitHub] flink issue #3534: [FLINK-6018][statebackend] Properly initialise StateDescr...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3534 @aljoscha Can you share https://github.com/aljoscha/flink/tree/jira-6018-state-init-fixups with @tzulitai who is looking into serializer upgrade paths? --- If your project is set up

[GitHub] flink issue #3534: [FLINK-6018][statebackend] Properly initialise StateDescr...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3534 This change is probably uncritical: - It seems this code path was never executed (by chance) because the `RuntimeContext` pre-initializes state descriptors, and all the operators directly

[GitHub] flink issue #3478: Flink 4816 Executions failed from "DEPLOYING" should reta...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3478 This change currently fails all CI build. The main reason is assuming that checkpointing is always active. --- If your project is set up for it, you can reply to this email and have your reply

[GitHub] flink issue #3394: [FLINK-5810] [flip6] Introduce a hardened slot manager

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/3394 I have some suggested edits on top of this (not strictly tied to the changes here) in that commit: https://github.com/StephanEwen/incubator-flink/commit/910de21972992d0ed80f232f3a64bf107c6819f2

[GitHub] flink pull request #3394: [FLINK-5810] [flip6] Introduce a hardened slot man...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3394#discussion_r106507099 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManager.java --- @@ -21,519 +21,897 @@ import

[GitHub] flink pull request #3394: [FLINK-5810] [flip6] Introduce a hardened slot man...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3394#discussion_r106514810 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManager.java --- @@ -21,519 +21,897 @@ import

[GitHub] flink pull request #3394: [FLINK-5810] [flip6] Introduce a hardened slot man...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3394#discussion_r106515494 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManager.java --- @@ -21,519 +21,897 @@ import

[GitHub] flink pull request #3394: [FLINK-5810] [flip6] Introduce a hardened slot man...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3394#discussion_r106508907 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/slotmanager/SlotManager.java --- @@ -21,519 +21,897 @@ import

[GitHub] flink pull request #3394: [FLINK-5810] [flip6] Introduce a hardened slot man...

2017-03-16 Thread StephanEwen
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/3394#discussion_r106458045 --- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/clusterframework/types/TaskManagerSlot.java --- @@ -18,15 +18,17 @@ package

<    5   6   7   8   9   10   11   12   13   14   >