[
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
