[
https://issues.apache.org/jira/browse/YARN-679?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15453458#comment-15453458
]
Daniel Templeton commented on YARN-679:
---------------------------------------
Continuing:
* The hyphen should be "that": {code} * Handler of interrupts -relays them to a
registered{code}
* This: {code} * Run a service -called after {@link Service#start()}.{code}
should be {code}+ * Run a service. This method is called after {@link
Service#start()}.{code}
* The period should be a colon here: {code} * <li>Any other exception. A
new {@link ServiceLaunchException} is created{code}
* This hyphen should be a dash or a comma: {code} * the GenericOptionsParser
-simply extracted to constants.{code}
* Same here: {code} * except in the case that the class does load but it
isn't actually{code}
* The {{@value }} tags in the {{LauncherArguments.ARG_CONFCLASS}} and
{{LauncherArguments. E_PARSE_FAILED}} javadocs are just kinda dangling out
there, not really adding anything--except maybe confusion.
* The javadoc here: {code}LauncherExitCodes.EXIT_FAIL{code} doesn't match the
pattern for the rest of the class' constants.
* The 40x/50x comments in the {{LauncherExitCodes}} class' constants' javadoc
need a little context around them. Otherwise it just seems like technical
Tourettes.
* The hyphen here should be a dash: {code} * <li>If any exception is raised
and provides an exit code
* -that is, it implements {@link ExitCodeProvider},{code}
* "configurations" should be possessive, not plural: {code} * is wrong and
logger configurations not on it, then no error messages by{code}
* In {{ServiceLauncher.launchServiceAndExit()}}, this bit {code} for (String
arg : args) {
builder.append('"').append(arg).append("\" ");
}{code} could be pulling out into another method and run lazily since it's
only needed in exceptional cases. You could also reuse it in
{{parseCommandArgs()}}.
* The first sentence of a javadoc header should summarize the content. This
one doesn't: {code} /**
* An exception has been raised.
* Save it to {@link #serviceException}, with the exit code in
* {@link #serviceExitCode}
* @param exitException exception
*/{code}
* Remove the period: {code} * @return the new options.{code}
* The hyphen here should be a dash or "because": {code} + "- it is
not a Configuration class/subclass");{code}
* The hyphen here should be a period or semicolon: {code}
LOG.debug("Failed to load {} -it is not on the classpath", classname);{code}
* "LaunchedService" should be "LaunchableService": {code} // it's a
launchedService, pass in the conf and arguments before init)
LOG.debug("Service {} implements LaunchedService", name);
launchableService = (LaunchableService) service;
if (launchableService.isInState(Service.STATE.INITED)) {
LOG.warn("LaunchedService {}"
{code}
* Won't this lead to confusing exceptions with stack traces as their messages
followed by their own stack traces? {code} if (message == null) {
// some exceptions do not have a message; fall back
// to the string value.
message = thrown.toString();
}{code}
* The javadoc for {{ServiceLauncher. registerFailureHandling()}} omits that it
also registers a handler for SIGTERM.
* The "Override point:" note seems to be missing from many of the
{{ServiceLauncher}} methods' javadoc.
* This should be in the check to see if logging is enabled: {code}
LOG.debug("Command line: {}", argString);{code}
I have to be honest; my eyes glazed over by the end, and I'm sure the quality
of my review suffered. The only thing that kept me going was the comforting
thought that you'll have as much fun sorting through my litany of comments as I
did digging through all that code.
Would it be possible to break this down into a few smaller patches? It would
help tremendously in getting it reviewed.
> add an entry point that can start any Yarn service
> --------------------------------------------------
>
> Key: YARN-679
> URL: https://issues.apache.org/jira/browse/YARN-679
> Project: Hadoop YARN
> Issue Type: New Feature
> Components: api
> Affects Versions: 2.8.0
> Reporter: Steve Loughran
> Assignee: Steve Loughran
> Attachments: YARN-679-001.patch, YARN-679-002.patch,
> YARN-679-002.patch, YARN-679-003.patch, YARN-679-004.patch,
> YARN-679-005.patch, YARN-679-006.patch, YARN-679-007.patch,
> YARN-679-008.patch, YARN-679-009.patch, YARN-679-010.patch,
> YARN-679-011.patch, org.apache.hadoop.servic...mon 3.0.0-SNAPSHOT API).pdf
>
> Time Spent: 72h
> Remaining Estimate: 0h
>
> There's no need to write separate .main classes for every Yarn service, given
> that the startup mechanism should be identical: create, init, start, wait for
> stopped -with an interrupt handler to trigger a clean shutdown on a control-c
> interrupt.
> Provide one that takes any classname, and a list of config files/options
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
