----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20241/#review40137 -----------------------------------------------------------
twill-core/src/main/java/org/apache/twill/internal/ContainerLiveNodeData.java <https://reviews.apache.org/r/20241/#comment73004> Better call this(containerId, host, null); twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java <https://reviews.apache.org/r/20241/#comment73006> The -Xdebug -Xrunjdwp is the old option for Java 1.4. Since Java 5, it's better to use -agentlib:jdwp=transport=dt_socket.... twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java <https://reviews.apache.org/r/20241/#comment73007> I think it needs volatile here, since the instanceNodeUpdate() method would get triggered from the ZK event thread while the getLiveNodeData() method could get called from any thread. twill-core/src/main/java/org/apache/twill/internal/json/JvmOptionsCodec.java <https://reviews.apache.org/r/20241/#comment73008> Do we need custom SerDe? Both JvmOptions and DebugOptions only has standard fields types which Gson already supported by default. Or is it for future expansion? twill-core/src/main/java/org/apache/twill/launcher/FindFreePort.java <https://reviews.apache.org/r/20241/#comment73009> What would be the behavior of execution of the runnable if this method throw exception? I suppose the runnable should failed to start? I am just not sure if that's the case in Shell. If it's been tested and verified, that it should be fine. twill-core/src/test/java/org/apache/twill/internal/json/JvmOptionsCodecTest.java <https://reviews.apache.org/r/20241/#comment73014> Missing License header. twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerMain.java <https://reviews.apache.org/r/20241/#comment73010> Why need to final all these? twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerService.java <https://reviews.apache.org/r/20241/#comment73011> Would be simpler if always call the three arguments constructor since it supports debugPort = null. twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java <https://reviews.apache.org/r/20241/#comment73012> Why it becomes non-public? twill-yarn/src/test/java/org/apache/twill/yarn/DebugTestRun.java <https://reviews.apache.org/r/20241/#comment73013> Missing License header. You can verify all files by running mvn apache-rat:check - Terence Yim On April 10, 2014, 11:48 p.m., Andreas Neumann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/20241/ > ----------------------------------------------------------- > > (Updated April 10, 2014, 11:48 p.m.) > > > Review request for Twill and Terence Yim. > > > Bugs: TWILL-70 > https://issues.apache.org/jira/browse/TWILL-70 > > > Repository: twill > > > Description > ------- > > - adds a new enableDebugging() to YarnTwillRunnerService > - adds debug options to the jvm options (that was a string, is now a > structure) > - changes the container launcher to find a free port and start the JVM with > that debug > - container main will now add the debug port to its live info > - app master watches that and accounts for the debug port in the resource > report > - controller yields debug port to client in resource report. > > > Diffs > ----- > > .gitignore 7aff00d52632ebcddd7081d6226777ad6b122ef9 > twill-api/src/main/java/org/apache/twill/api/TwillRunResources.java > 4c3d2e79a19ad8ed4cc04464844832e7202a907d > > twill-api/src/main/java/org/apache/twill/internal/DefaultTwillRunResources.java > bd8f8f52293f452a952a6736d8dfb42c1214fe0a > > twill-core/src/main/java/org/apache/twill/internal/ContainerLiveNodeData.java > 705943c9b67cffa41836dfc32a7fb1b8dd65db8c > twill-core/src/main/java/org/apache/twill/internal/JvmOptions.java > PRE-CREATION > > twill-core/src/main/java/org/apache/twill/internal/TwillContainerController.java > bb46cd5e8fc857a4e3739c3915228ad271012f3e > > twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java > 430e63ac126639c67ec07cf6f2bf982a5e9688df > > twill-core/src/main/java/org/apache/twill/internal/json/JvmOptionsCodec.java > PRE-CREATION > > twill-core/src/main/java/org/apache/twill/internal/json/TwillRunResourcesCodec.java > c39fa16c2bbdba7318eaac1f15355e6f046b6636 > twill-core/src/main/java/org/apache/twill/launcher/FindFreePort.java > PRE-CREATION > > twill-core/src/test/java/org/apache/twill/internal/json/JvmOptionsCodecTest.java > PRE-CREATION > > twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java > 3a6ce2027a941da652ee7d8d4e2195b220dfdeb3 > > twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java > 63e3db864a95319624f83012962dff53ce0f60b2 > > twill-yarn/src/main/java/org/apache/twill/internal/appmaster/TrackerService.java > a9553c9b336642f57d579db08674b1bc3da9e30c > > twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerMain.java > c3aece691fb5d5f388c5b316a79b911ca7421e58 > > twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerService.java > 9890f1765b794b443a006c09087eeb0877c76bfd > twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java > 8c9662920ed7cf3ca3821c0519468afeee5faa2b > twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java > 72ea58be1f2828b3e311e492fbd4cf10e6ed0fe4 > twill-yarn/src/test/java/org/apache/twill/yarn/DebugTestRun.java > PRE-CREATION > twill-yarn/src/test/java/org/apache/twill/yarn/SessionExpireTestRun.java > ec290614831cc1c2ae1275df45d2ec0ef89017cb > twill-yarn/src/test/java/org/apache/twill/yarn/YarnTestSuite.java > 51b6abfbf62901720b1496a4e45331cf9e561bee > > Diff: https://reviews.apache.org/r/20241/diff/ > > > Testing > ------- > > New unit test is DebugTestRun. Also did quite some manual testing. > > > Thanks, > > Andreas Neumann > >
