Github user squito commented on the pull request:

    https://github.com/apache/spark/pull/5792#issuecomment-101783127
  
    Hi @harishreedharan, I left a couple of comments, but some more high-level 
comments:
    
    * right now this lives in a separate Servlet, and is served in paths 
similar to the UI.  Now that we have a separate servlet for the rest api, woudl 
it make more sense to combine it with that?  Eg., add another path here: 
https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/status/api/v1/JsonRootResource.scala#L49
    for `@Path(applications/{appId}/{attemptId}/eventLogs)`.  I think you can 
also get jersey to do the content-type filtering for you.  This might require a 
little more refactoring to those utils, and perhaps some renaming (eg., 
`JsonRootResource` maybe should really be `RestRootResource`).
    * can you add some tests?  You should be able to add to HistoryServerSuite 
-- there are already some event logs available as test resources


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to