> On April 11, 2014, 6:18 a.m., Terence Yim wrote: > > twill-core/src/main/java/org/apache/twill/internal/TwillContainerLauncher.java, > > line 140 > > <https://reviews.apache.org/r/20241/diff/1/?file=554880#file554880line140> > > > > The -Xdebug -Xrunjdwp is the old option for Java 1.4. Since Java 5, > > it's better to use > > > > -agentlib:jdwp=transport=dt_socket....
I picked this because it seems to be the consensus in some forums that it works for all JVMs. I am happy to change it. > On April 11, 2014, 6:18 a.m., Terence Yim wrote: > > twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerMain.java, > > line 77 > > <https://reviews.apache.org/r/20241/diff/1/?file=554888#file554888line77> > > > > Why need to final all these? Not sure. I did not make that change. Perhaps IntelliJ is smarter than I and did that for me... Myself I did not even touch these. > On April 11, 2014, 6:18 a.m., Terence Yim wrote: > > twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillRunnerService.java, > > line 172 > > <https://reviews.apache.org/r/20241/diff/1/?file=554891#file554891line172> > > > > Why it becomes non-public? I don't know. Don't think I removed the public... I am starting to distrust IntelliJ 13. > On April 11, 2014, 6:18 a.m., Terence Yim wrote: > > twill-yarn/src/test/java/org/apache/twill/yarn/DebugTestRun.java, line 1 > > <https://reviews.apache.org/r/20241/diff/1/?file=554892#file554892line1> > > > > Missing License header. You can verify all files by running > > > > mvn apache-rat:check I also fixed three more found by apache-rat:check. why don't we run that as part of every build? Just like we do for checkstyle? > On April 11, 2014, 6:18 a.m., Terence Yim wrote: > > twill-core/src/main/java/org/apache/twill/internal/json/JvmOptionsCodec.java, > > line 28 > > <https://reviews.apache.org/r/20241/diff/1/?file=554881#file554881line28> > > > > 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? I simply followed the way that it has been done for other classes. I was wondering myself whether it is really needed. I guess one advantage is that the custom serde can return immutable objects. > On April 11, 2014, 6:18 a.m., Terence Yim wrote: > > twill-core/src/main/java/org/apache/twill/launcher/FindFreePort.java, line > > 10 > > <https://reviews.apache.org/r/20241/diff/1/?file=554883#file554883line10> > > > > 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. The program would fail, and the value of the TWILL_DEBUG_PORT would not be a number, so the attempt to start the container would also fail. I tested this manually/involuntariy... I will use && instead of ; to compose the two commands. That will abort if the first command fails. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/20241/#review40137 ----------------------------------------------------------- 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 > >
