Thanks for the review. Junping Du
1. For generating URI embed in response of getContainerLogsInfo, I saw some very similar one in other places, like: getLogs(). Can we refactor the code a bit to reuse the same logic?
Yes, other than this, we have lots of duplicate codes related to webservice. Create a separate jira for the refactory work: https://issues.apache.org/jira/browse/YARN-6080
Also added the TODO for this.
2. For getContainerLogsInfo(), if an app is not in running or finished state, here we will return bad request. However, I remember in our ATS implementation, RM after restart could send regressioned application state event to ATS, like app creation event to ATS which was running before. Can you double check ATS's app status won't have regression? Otherwise, we shouldn't just simply return a bad request.
This is fine. Based on how we generate ApplicationReport from ATS entities, the CREATE event will only provide the created time for the application. The application state is generated based on ApplicationMetricsConstants.STATE_EVENT_INFO which is provided by ApplicationFinishedEvent and ApplicationStateUpdatedEvent. For the ApplicationStateUpdatedEvent, we only submit the event when the app transits from ACCEPTED to RUNNING state.
So, when the Application is in RUNNING state in RM, the state will be RUNNING in ATS. Event if the RM restart/ AM restart happens later, the state will not be changed. So, I think that right now, it is fine that we only check for the RUNNING state here .
3. For getContainerLogMeta(), I remember I have some previous comments on refactor code (consolidate similar logic, especially log reader) in previous JIRAs. How's going with that effort? If that effort is not a short term priory for you, please add a TODO here - may be someone else read this part of code could help on that.
Link the jira: https://issues.apache.org/jira/browse/YARN-4993
Added the TODO for this.