[
https://issues.apache.org/jira/browse/YARN-3264?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14346355#comment-14346355
]
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.
{code}
98 /**
99 * Error code returned when no start time can be found when putting
an
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;
{code}
2. For the POC implementation config, IMOH, it's better to be put in the writer
impl. YarnConfiguration is the API realm.
{code}
1253 /** Default PoC value for timeline service storage tmp root for FILE
YARN-3264 */
1254 public static final String TIMELINE_SERVICE_STORAGE_DIR_ROOT =
"/tmp/";
{code}
3. Move the cleanup code into the finally block? Hence even the assertion
failure will not leave uncleaned files.
{code}
359 // remove the files and dirs if empty
{code}
4. Make it private?
{code}
TimelineServiceWriter writer ;
{code}
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
{code}
private final String TMP_ROOT = "/tmp/";
{code}
6. Some minor issue about class name and variable name
{code}
52 * are to be done are given by {@link AggregateUpTo}
{code}
{code}
65 TimelineAggregationGranularity aggregateUpTo) throws IOException;
{code}
7. No need to declare JsonGenerationException and JsonMappingException.
{code}
101 TimelineServiceWriteResponse response) throws
JsonGenerationException,
102 JsonMappingException, IOException {
{code}
8. Can we change it to debug level and put in a {{LOG.isDebugEnabled()}} block?
{code}
LOG.trace("SUCCESS - TIMELINE V2 PROTOTYPE");
90 LOG.trace("postEntities(entities=" + entities + ", callerUgi=" +
79 callerUgi + ")"); 91 callerUgi + ")");
{code}
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.
{code}
String appTimestampFileName = appId.replace("application_", "appAttempt_");
{code}
> [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
(v6.3.4#6332)
