[
https://issues.apache.org/jira/browse/YARN-4025?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14701309#comment-14701309
]
Junping Du commented on YARN-4025:
----------------------------------
Thanks [~sjlee0] for updating the patch. 003 patch looks good to me in general.
Some minor comments:
In ColumnHelper.java,
{code}
/**
...
+ * @param columnPrefixBytes
+ * optional prefix to limit columns. If null all columns are
+ * returned.
...
+ */
+ public Map<byte[][], Object> readResultsHavingCompoundColumnQualifiers(
{code}
Do we handle columnPrefixBytes to be null here as Javadoc comments? I saw we
handle this case explicitly in readResults() but I didn't see it here. Let me
know if I miss something. In addition, looks like previous readResults() only
handle the case CQs are all String. I think we should update that method name
to something like: readResultsWithAllStringColumnQualifiers() to get rid of
possible confusion. Last but not the least, for result to be null case, do we
need to handle it with log some warn messages like other cases in this patch?
{code}
+ byte[][] columnQualifierParts = Separator.VALUES.split(
+ columnNameParts[1], -1);
{code}
Checking with javadoc in Separator and TimelineWriterUtils - "a negative value
indicates no limit on number of segments.", so can we define a constant value
like NO_LIMIT to replace -1 here? Actually, from checking with implementation
in TimelineWriterUtils, 0 also indicates the same thing (no limit). Sounds like
we don't have any tests in TestTimelineWriterUtils.java, we may want to improve
this in future?
In ApplicationTable,
{code}
- * | | e!eventId?timestamp?infoKey: | | |
+ * | | e!eventId=timestamp=infoKey: |
{code}
I think we should do the same thing to some javadoc examples in
EntityTable.java.
Other looks fine to me.
> Deal with byte representations of Longs in writer code
> ------------------------------------------------------
>
> Key: YARN-4025
> URL: https://issues.apache.org/jira/browse/YARN-4025
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: timelineserver
> Reporter: Vrushali C
> Assignee: Sangjin Lee
> Attachments: YARN-4025-YARN-2928.001.patch,
> YARN-4025-YARN-2928.002.patch, YARN-4025-YARN-2928.003.patch
>
>
> Timestamps are being stored as Longs in hbase by the HBaseTimelineWriterImpl
> code. There seem to be some places in the code where there are conversions
> between Long to byte[] to String for easier argument passing between function
> calls. Then these values end up being converted back to byte[] while storing
> in hbase.
> It would be better to pass around byte[] or the Longs themselves as
> applicable.
> This may result in some api changes (store function) as well in adding a few
> more function calls like getColumnQualifier which accepts a pre-encoded byte
> array. It will be in addition to the existing api which accepts a String and
> the ColumnHelper to return a byte[] column name instead of a String one.
> Filing jira to track these changes.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
