[GitHub] flink pull request #6370: [FLINK-9894] [runtime] Potential Data Race

2018-07-19 Thread tison1
Github user tison1 closed the pull request at: https://github.com/apache/flink/pull/6370 ---

[GitHub] flink issue #6370: [FLINK-9894] [runtime] Potential Data Race

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6370 @zentol ok ... close as suggested, would be resolved in #6353 ---

[GitHub] flink pull request #:

2018-07-19 Thread tison1
Github user tison1 commented on the pull request: https://github.com/apache/flink/commit/8231b62ff42aae53ca3a7b552980838ccab824ab#commitcomment-29765803 In flink-runtime/src/main/java/org/apache/flink/runtime/jobmanager/scheduler/CoLocationGroup.java: In flink-runtime/src/main

[GitHub] flink issue #6367: [FLINK-9850] Add a string to the print method to identify...

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6367 @yanghua also +1 this is a net win. ---

[GitHub] flink issue #6370: [FLINK-9894] [runtime] Potential Data Race

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6370 but the original `ensureConstraints` is wired. For example it calls `ensureCapacity` twice and the only code path is from `ExecutionJobVertex` construct `ExecutionVertex` which calls

[GitHub] flink issue #6370: [FLINK-9894] [runtime] Potential Data Race

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6370 @yanghua AFAIK, yes. ---

[GitHub] flink issue #6360: [FLINK-9884] [runtime] fix slot request may not be remove...

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6360 > When task executor report a slotA with allocationId1, it may happen that slot manager record slotA is assigned to allocationId2, and the slot request with allocationId1 is not assigned. Then s

[GitHub] flink pull request #6367: [FLINK-9850] Add a string to the print method to i...

2018-07-19 Thread tison1
Github user tison1 commented on a diff in the pull request: https://github.com/apache/flink/pull/6367#discussion_r203644208 --- Diff: flink-streaming-java/src/main/java/org/apache/flink/streaming/api/functions/sink/PrintSinkFunction.java --- @@ -40,6 +40,8 @@ private

[GitHub] flink issue #6370: [FLINK-9894] [runtime] Potential Data Race

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6370 by analyses the use path of this method, we gain little on `ensureCapacity`, in fact, test fails on #6353 is caused by too many `ensureCapacity` and then `Array.copyOf` race each other. ---

[GitHub] flink pull request #6370: [FLINK-9894] [runtime] Potential Data Race

2018-07-19 Thread tison1
GitHub user tison1 opened a pull request: https://github.com/apache/flink/pull/6370 [FLINK-9894] [runtime] Potential Data Race ## What is the purpose of the change *(CoLocationGroup#ensureConstraints may cause data race on `constraints`. synchronize its use to avoid

[GitHub] flink issue #6354: [FLINK-9881] Fixed a typo in table.scala

2018-07-19 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6354 @twalthr thank you for the hint. I've updated my PRs #6353 #6345 #6339 as you suggested. ---

[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...

2018-07-18 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6353 Fix unstable case, the problem is code below, that may assign `constraints` to a long array and then to a short array, which cause out of index exception. to solve it we could init `constraints

[GitHub] flink issue #6364: [hotfix] typo for SqlExecutionException msg

2018-07-18 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6364 Thanks! LGTM cc @zentol ---

[GitHub] flink issue #6360: [FLINK-9884] [runtime] fix slot request may not be remove...

2018-07-18 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6360 @shuai-xu It makes sense. The message that TM has successfully allocated slot might lost in transport. When slot manager receives a slot status report which says one slot has allocation

[GitHub] flink issue #6345: [FLINK-9869] Send PartitionInfo in batch to Improve perfo...

2018-07-18 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6345 OK. This PR is about performance improvement. I will try to give out a benchmark, but since it is inspired by our own batch table tasks, it might take time to give one. Though since this PR

[GitHub] flink issue #6358: [FLINK-9882] [runtime] A function access can be private

2018-07-18 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6358 LGTM ---

[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...

2018-07-18 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6353 This pr cause `HITSITCase` unstable, I retrigger ci two times to get more info going to fix it. ---

[GitHub] flink issue #6339: [FLINK-9859][Runtime] Distinguish TM akka config with JM ...

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6339 cc @StephanEwen ---

[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6353 I think `ExecutionGraphConstructionTest` covers it. ---

[GitHub] flink issue #6354: [FLINK-9881] Fixed a typo in table.scala

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6354 LGTM! It's a net win. ---

[GitHub] flink issue #6353: [FLINK-9875] Add concurrent creation of execution job ver...

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6353 the existing tests verify correctness. I will take a try to give out a benchmark report since this PR is more relevant to performance. ---

[GitHub] flink issue #6345: [FLINK-9869] Send PartitionInfo in batch to Improve perfo...

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6345 cc @sihuazhou ---

[GitHub] flink pull request #6353: [FLINK-9875] Add concurrent creation of execution ...

2018-07-17 Thread tison1
GitHub user tison1 opened a pull request: https://github.com/apache/flink/pull/6353 [FLINK-9875] Add concurrent creation of execution job vertex ## Add concurrent creation of execution job vertex in some case like inputformat vertex, creation of execution job vertex

[GitHub] flink pull request #6347: [hotfix] consistency: vertexes -> vertices

2018-07-17 Thread tison1
Github user tison1 closed the pull request at: https://github.com/apache/flink/pull/6347 ---

[GitHub] flink pull request #6347: [hotfix] consistency: vertexes -> vertices

2018-07-17 Thread tison1
Github user tison1 closed the pull request at: https://github.com/apache/flink/pull/6347 ---

[GitHub] flink issue #6347: [hotfix] consistency: vertexes -> vertices

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6347 > vertices is the correct plural, but this is another one of those cases where fixing it might cause more harm than good since it could cause merge conflicts, yet provides no functional bene

[GitHub] flink issue #6347: [hotfix] consistency: vertexes -> vertices

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6347 > vertices is the correct plural, but this is another one of those cases where fixing it might cause more harm than good since it could cause merge conflicts, yet provides no functional bene

[GitHub] flink issue #6347: [hotfix] consistency: vertexes -> vertices

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6347 > Additionally this PR makes a lot of whitespace changes that should be reverted in any case. did you mean the whitespace in comment `* ` is significant? ---

[GitHub] flink issue #6347: [hotfix] typo: vertexes -> vertices

2018-07-17 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6347 look up dictionary and it says "vertexes" is also vertex pluralities :P but the most of our code use "vertices", so could this PR thought as keeping consistency? ---

[GitHub] flink pull request #6347: [hotfix] typo: vertexes -> vertices

2018-07-17 Thread tison1
GitHub user tison1 opened a pull request: https://github.com/apache/flink/pull/6347 [hotfix] typo: vertexes -> vertices cc @zentol @yanghua You can merge this pull request into a Git repository by running: $ git pull https://github.com/tison1/flink master Alternatively

[GitHub] flink issue #6345: [FLINK-9869] Send PartitionInfo in batch to Improve perfo...

2018-07-16 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6345 cc @tillrohrmann @fhueske ---

[GitHub] flink pull request #6345: [FLINK-9869] Send PartitionInfo in batch to Improv...

2018-07-16 Thread tison1
GitHub user tison1 opened a pull request: https://github.com/apache/flink/pull/6345 [FLINK-9869] Send PartitionInfo in batch to Improve perfornance ## What is the purpose of the change Current we send partition info as soon as one arrive. we could `cachePartitionInfo

[GitHub] flink issue #6339: [FLINK-9859][Runtime] Distinguish TM akka config with JM ...

2018-07-16 Thread tison1
Github user tison1 commented on the issue: https://github.com/apache/flink/pull/6339 cc @zentol @tzulitai ---

[GitHub] flink pull request #6339: [FLINK-9859][Runtime] Distinguish TM akka config w...

2018-07-16 Thread tison1
GitHub user tison1 opened a pull request: https://github.com/apache/flink/pull/6339 [FLINK-9859][Runtime] Distinguish TM akka config with JM config ## What is the purpose of the change Distinguish TM akka config with JM config. 1. increase the number of akka