hadoop-yarn-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Daniel Templeton (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (YARN-679) add an entry point that can start any Yarn service
Date Fri, 26 Aug 2016 21:34:20 GMT

    [ https://issues.apache.org/jira/browse/YARN-679?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15439909#comment-15439909
] 

Daniel Templeton commented on YARN-679:
---------------------------------------

I made it a bit farther.  Additional comments:

* I see that the pattern of the code from the cause trumping the code parameter shows up in
several other places.  I see what's you're going for, but I really dislike the idea of passing
in two arguments for the same data with one being ignored.  I'm open to discuss it.
* Should the {{ServiceStateException}} and {{ExitUtil.ExitException}} have a constructor that
only takes a message and exit code?
* Shouldn't need the period after {code}* {@inheritDoc}.{code}
* I'd rather see {code}  private static final Logger LOG = LoggerFactory.getLogger(
      HadoopUncaughtExceptionHandler.class);{code} break on the equals instead of the paren.

Here are also some language issues from the package docs:

* Should have an "and" instead of a comma: {code} down the CLI arguments, to execute an operation
without having to{code}
* Should be "out" not "our": {code}exit if it times our or a second interrupt is received.{code}
* I'd rephrase: {code}It will instantiate the service classname provided, by zero-args constructor.
Or, if none is available, falling back to a constructor that takes a {@code String} as its
parameter, on the assumption that the parameter is the service name.{code} as {code}It will
instantiate the service classname provided, using the no-args constructor, or if no such constructor
is available, it will fall back to a constructor with a single {@code String} parameter, passing
the service name as the parameter value.{code}
* Missing a close paren before the comma here: {code}{@code STATE.STOPPED}, escalated into
a non-zero return code{code}
* Extra space before the dash (and add another hyphen to make it a dash or use {{&mdash;}}):
{code}(prepare configuration files -covered later){code}.
* Same here: {code}and control-C signals -calling {@code stop()} on the service when signalled{code}
* And here: {code}parse the command line -it just becomes the responsibility of the{code}
* And here: {code}(prepare configuration files --covered later){code}
* Here the hyphen should be a comma: {code}and start it -triggering shutdown when signalled{code}
* Same here: {code}It adds two methods to the service interface -and hence two new features{code},
though I suppose a dash could make sense...
* And here: {code}returned by the method -so services may signal failures simply by returning{code}
* And here: {code}Exceptions are converted into exit codes -but rather than simply{code}
* "Signalled" is misspelled several times.  Should be "signaled"
* {code}(there is way covered later){code} is missing an "a"
* Here: {code}commands can be implemented as such services, so integrating with YARN's{code},
the "so" should be a "thus"
* Same here: {code}initialized, so allowing services to tune their configuration data before{code}
* This hyphen is spurious: {code}-which may be needed to trigger the injection of HDFS or
YARN resources{code}
* Same here: {code}of returning error codes to signal failures -and for {code}
* And here: {code}these classes -simply to force in the common resources{code}
* And here: {code}files are merged -with the latest file on the command line being the{code}
* Remove the space before the comma: {code}have a "something went wrong" exit code , exceptions
<i>may</i>{code}
* "such" should be "the": {code}the service instance and such like{code}
* Missing an "i" in "is": {code}exit code of 0 s created{code}
* Here the "-so" should just be a comma: {code}stop the service if a shutdown request is received
-so ensuring that{code}

Maybe after the weekend I'll be up to finishing the review. :)

> 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: yarn-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: yarn-issues-help@hadoop.apache.org


Mime
View raw message