hadoop-mapreduce-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Graves (Commented) (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (MAPREDUCE-2863) Support web-services for RM & NM
Date Tue, 29 Nov 2011 16:53:44 GMT

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

Thomas Graves commented on MAPREDUCE-2863:
------------------------------------------

Hitesh - thanks for the review!

output format is controlled by the accept header.  If not specified it currently defaults
to xml - perhaps json would be a better default here since I see more people using it - preference?
 There is no way to control it via query parameter right now - any specific reason you can't
use accept header?

I'll run through the code again and remove the extra commented out code. I left some of the
it in there because I planned on adding it back in in a followup jira(s) but will go ahead
and remove.

In this case most of the class member variables could be made final - in general though most
of the time the jaxb classes have both getters and setters and wouldn't be final.  I know
its a bit quirky having the constructors due all the initialization but it was a convenient
way to commonize that code between the html and ws without adding yet more classes.   I don't
see it being necessary as there is no harm if someone did want to set one after the constructor
but will if you prefer?  
I'm hoping we can do more commonizing of things between the html and ws but thought this would
be a decent first step.

{quote}
NMWebServices#getNodeApps
    Is the null check required? Likewise for other cases where an info object is initialized
via its constructor.
{quote}
No, not required just me being paranoid when I first wrote that.  Will remove and in other
classes where isn't needed.

{quote}
TestRMWebServices#verifyClusterInfoXML

    remove dead code - NodeList nl = docEle.getElementsByTagName("Employee");
    the test needs to be enhanced to match the json checks.
    using any converter api from xml to json or vice versa, a single content verification
function can be used to verify both outputs as well as check consistency in elements across
the two formats.
{quote}
Left the code there for follow up jira to add more unit tests. Will remove.
Great idea on the converter!  unless objections I would like to do that in a follow on jira.

{quote}
TestRMWebServices
    matching the version info - this can potentially also be done by getting the version from
the environment by having a system property be set. See pom for yarn.applications.distributedshell
or mapreduce-jobclient.
{quote}
Ok will change.

{quote}
RMWebServices#hasAccess

    my knowledge on hadoop security is still limited but do we need to worry about remoteUser
or callerugi being null in a secure setup? If yes, a null value should make hasAccess return
false in such a setup.
{quote}
The remoteUser could be null if not going through a filter - although hadoop defaults to a
user (Dr.Who or webuser via StaticUserWebFilter) so I don't think its really possible.  The
only case might be if someone creates there own filter and somehow doesn't set it - not really
sure if that is possible.  That code was copied directly from the html code but I see its
even inconsistent between RM/AM so will look into this code some more.

{quote}
RMWebServices#getNodeInfo

    with reference to 'nodeId.split(":")' , you may need to confirm that all query params
get url decoded automatically.
{quote}
Jersey handles decoded the query parameters for you. For instance if you pass in nodeid like:
host%3A45454 - it works fine. Will make sure to add tests for this.  Did you have any specific
examples in mind for the nodeId?  Note I also switched to use the ConverterUtils for this
which I somehow missed the first time around when I looked for it.

{quote}
RMWebServices#getApps

    check for invalid values? count=0/negative time vals/fEnd > fBegin?
    minor optimization - s/app.getStartTime() < sBegin || app.getStartTime() > sEnd/app.getStartTime()
>= sBegin && app.getStartTime <= sEnd/
    remove commented out code.
    the user and queue name seems to be compared without case sensitivity. Is this correct?
    inconsistency in handling of secure access. getApps hides away some information if certain
apps are not accessible whereas getApp throws an exception.
{quote}

I don't follow the optimization of the starttime - that is a continue statement and hence
the ||. if I switch it then it will skip it if it is what you asked for?

The inconsistent handling was intentional.  It mirrors the html right now.  I had thought
about it returning subset of data in both cases - is that what you prefer?  In my opinion
that entire thing is a bit weird how that works. Wish there was a better way of letting user
know - any ideas?

{quote}
DefaultSchedulerInfo

    Could we just do this.qstate = qInfo.getQueueState().name ?
    should this be called FifoSchedulerInfo instead of Default? It gives an assumption that
fifo will always be the default.
{quote}

The difference in the qstate is the Undefined gets mapped to Unknown. I don't have strong
preference one way or another it was again copied from html.  We can probably just have it
print out the undefined - let me know if you want me to change it.
Sure, will rename it.

{quote}
JobHistoryParser

    Not sure about legacy tools but would something like changing default val of errorInfo
in jobhistory from "None" to "" affect things?
{quote}
I didn't see it used anywhere in 20 security branch. The job history UI on that has "Failure
Info" field but it is blank if job succeeded.  In the 20 JobStatus is actually sets the failureInfo
to "NA" by default so I guess if your job failed and the failure info wasn't provided it would
print NA. I figured showing "None" vs empty field was the same but I guess if it failed and
no other info is available None might be more user friendly then empty field?

{quote}
(Job)HsWebServices:

    @Path("/ws/v1/history")
    should this be change to be more specific to denote MR job history?
    secure access? This does not seem to enforce any checks. Again, just a question to bridge
my knowledge gap.
{quote}
The path was intentional since there is a jira out there to make job history more generic
and handle things other then MR.  Everything mapreduce related is under history/mapreduce/.
 If you see better way let me know.   
The checks for security seem to be a level up in the CompletedJob and similar code but as
stated above security in AM and history is broken and jira is filed for that so will handle
fixing any issues with that since I have no way to test right now.

{quote}
AMWebServices#getJobTaskAttemptId

    This function and a couple of others return a null when data is not found as compared
to NotFoundExceptions in other cases. Is null handled as a 404 by the framework similar to
NotFoundException ?
{quote}
returning null returns a - HTTP/1.1 204 No Content.  Will change it to be consistent with
the rest.


Everything else I didn't comment on is fixed as requested. 

                
> Support web-services for RM & NM
> --------------------------------
>
>                 Key: MAPREDUCE-2863
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-2863
>             Project: Hadoop Map/Reduce
>          Issue Type: Improvement
>          Components: mrv2, nodemanager, resourcemanager
>    Affects Versions: 0.23.0
>            Reporter: Arun C Murthy
>            Assignee: Thomas Graves
>            Priority: Blocker
>         Attachments: MAPREDUCE-2863.patch, MAPREDUCE-2863.patch, MAPREDUCE-2863.patch,
amoutput.txt, appoutput.txt, hsoutput.txt, nmoutput.txt, nmoutput.txt, nmoutput.txt, rmoutput.txt,
rmoutput.txt, rmoutput.txt
>
>
> It will be very useful for RM and NM to support web-services to export json/xml.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Mime
View raw message