Zhijie Shen commented on YARN-3264:

Overall, I think the patch should work. Some more detailed comments about the 
patch. Please take a look.

1. I think NO_START_TIME may no longer make sense in ATS v2.
98          /**
99           * Error code returned when no start time can be found when putting 
100          * entity. This occurs when the entity does not already exist in 
the store
101          * and it is put with no start time or events specified.
102          */
103         public static final int NO_START_TIME = 1;

2. For the POC implementation config, IMOH, it's better to be put in the writer 
impl. YarnConfiguration is the API realm. 
1253      /** Default PoC value for timeline service storage tmp root for FILE 
YARN-3264 */
1254      public static final String TIMELINE_SERVICE_STORAGE_DIR_ROOT = 

3. Move the cleanup code into the finally block? Hence even the assertion 
failure will not leave uncleaned files.
359         // remove the files and dirs if empty

4. Make it private?
TimelineServiceWriter writer ;

5. Make it static? I suggest to have TIMELINE_SERVICE_STORAGE_ROOT_DIR as the 
config name = "yarn.timeline-service.fs-writer.root-dir" and have 
DEFAULT_TIMELINE_SERVICE_STORAGE_ROOT_DIR = "/tmp/timeline_service_data", or 
something similar
private final String TMP_ROOT = "/tmp/";

6. Some minor issue about class name and variable name
52         * are to be done are given by {@link AggregateUpTo}
65            TimelineAggregationGranularity aggregateUpTo) throws IOException;

7. No need to declare JsonGenerationException and JsonMappingException.
101           TimelineServiceWriteResponse response) throws 
102           JsonMappingException, IOException {

8. Can we change it to debug level and put in a {{LOG.isDebugEnabled()}} block?
            LOG.trace("SUCCESS - TIMELINE V2 PROTOTYPE");
90          LOG.trace("postEntities(entities=" + entities + ", callerUgi=" +
79              callerUgi + ")");       91              callerUgi + ")");

9. Shall we also declare ".thist" as a private static final String?

10. You can use ConverterUtils to convert string to app/attempt/container id 
object, and you can construct or get app/attempt/container id object from each 
other. It may help you here to cleanup the code.
String appTimestampFileName = appId.replace("application_", "appAttempt_");

> [Storage implementation] Create a POC only file based storage implementation 
> for ATS writes
> -------------------------------------------------------------------------------------------
>                 Key: YARN-3264
>                 URL: https://issues.apache.org/jira/browse/YARN-3264
>             Project: Hadoop YARN
>          Issue Type: Sub-task
>          Components: timelineserver
>            Reporter: Vrushali C
>            Assignee: Vrushali C
>         Attachments: YARN-3264.001.patch, YARN-3264.002.patch
> For the PoC, need to create a backend impl for file based storage of entities 

This message was sent by Atlassian JIRA

Reply via email to