Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/66
+1
LGTM
---
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/62
One question about change form using BufferedReader, but looks good.
If passes tests +1
---
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/62#discussion_r143848112
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/ResourceReportClient.java ---
@@ -52,12 +54,16 @@
public ResourceReport get
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107788346
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
YarnTwillController(String appName
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107781058
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/40
+1 for correctness
I just added suggestion of style of return null vs declarative check by
caller to determine if logging collection is disabled.
---
If your project is set up for it, you
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107771740
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107739693
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
---
@@ -105,4 +114,17 @@ public String getRmSchedulerAddr
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107690293
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java
---
@@ -71,21 +71,37 @@
private static final Logger LOG
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107689017
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
YarnTwillController(String appName
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107347185
--- Diff:
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillController.java ---
@@ -77,7 +77,7 @@
*/
YarnTwillController(String appName
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107346167
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java
---
@@ -71,21 +71,37 @@
private static final Logger LOG
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/40#discussion_r107344378
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java
---
@@ -71,21 +71,37 @@
private static final Logger LOG
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/35
+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 feature
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/35
Ah, yes, sorry, I thought I added the ignore white spaces when load the
files changes view.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/34
Ah ok
---
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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/33
@chtyim I was asking concern about the selection of double vs float
---
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 hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/33#discussion_r101708864
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java
---
@@ -59,19 +59,21 @@
private final int instanceCount
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/33
@yufeldman : Could you kindly add in the summary of the PR the summary of
the approach you are taking to fix the issue?
Will help with the review to compare expectation vs implementation
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/30
Cool! Looking forward to them =)
---
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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/30
Could you check your Git identity via `git config --global user.name` and
`git config --global user.email` ?
---
If your project is set up for it, you can reply to this email and have your
reply
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/30
Hi @serranom , I just noticed that the commits you made use different
account than your pull request. I would advise to check it to make sure your
contributions easily tracked in Github
---
If your
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/23
Looks like not related to this PR. Will merge EOD if no more comment.
Thanks again for the hard work, @serranom
---
If your project is set up for it, you can reply to this email and have
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/23
I am having problem applying the patch, could you kindly rebase the patch
into single commit?
Thanks
---
If your project is set up for it, you can reply to this email and have your
reply appear
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/23#discussion_r99416151
--- Diff:
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
---
@@ -477,10 +493,30 @@ void handleCompleted
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/30
Oh sorry, Terence already commented about merging to the Site. Pls continue
=P
---
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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/29
Will merge this if no more 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 this feature
enabled
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/25
@anew please close it if already 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 does not have this feature
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/30
I personally dont' mind and actually prefer the extra blank new line =)
---
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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/30
I dont remember if the check style will barf about it. I dont use Eclipse,
so could yo try if it pass them maven checkstyle?
---
If your project is set up for it, you can reply to this email
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/29
This LGTM
+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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/23
Let's trigger new build, @serranom could you close and reopen this PR
please?
---
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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/25
@anew feel free 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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/23
Hi @serranom , usually in about week you should get review. I apologize for
the delay, but sometimes there is delay just due to volunteer time.
As for CI failures, it depends, sometime ZK is flaky
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/23
Ah, I see you out the proposed design as JIRA comment.
Usually the design in JIRA changes but once you start implementing, that's
why I usually ask for developers to add the actual design or plan
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/23
Thanks for the PR. Could you kindly add more in the PR description on how
the solution is implemented.
This will help review to "compare" the code implementation with the
description o
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/12
Does TWILL-138 depends on TWILL-195 ?
---
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 hsaputra commented on the issue:
https://github.com/apache/twill/pull/10
Thanks for the PR, @gokulavasan. Next time would love to have more
information how you solve it in the PR description rather than just link to
JIRA.
---
If your project is set up for it, you can
Github user hsaputra commented on the issue:
https://github.com/apache/twill/pull/2
Hi @poornachandra , are you still working on the PR for updates, or ready
for review follow up?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub
Github user hsaputra commented on a diff in the pull request:
https://github.com/apache/twill/pull/2#discussion_r75424881
--- Diff:
twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java
---
@@ -153,21 +155,29 @@ public TwillContainerController start(RunId
40 matches
Mail list logo
