[
https://issues.apache.org/jira/browse/HDFS-9260?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15110816#comment-15110816
]
Staffan Friberg commented on HDFS-9260:
---------------------------------------
Thanks for the comments [~cmccabe]! Did all your suggested changes, except the
two below which needed some further discussion.
{quote}
{code}
> if (shouldPostponeBlocksFromFuture) {
> // If the block is an out-of-date generation stamp or state,
> // but we're the standby, we shouldn't treat it as corrupt,
> // but instead just queue it for later processing.
> // TODO: Pretty confident this should be s/storedBlock/block below,
> // since we should be postponing the info of the reported block, not
> // the stored block. See HDFS-6289 for more context.
> queueReportedBlock(storageInfo, storedBlock, reportedState,
> QUEUE_REASON_CORRUPT_STATE);
> } else {
{code}
If we're really confident that this should be "block" rather than
"storedBlock", let's fix it.
{quote}
This comment is simply copied from the method "processAndHandleReportedBlock"
in the same class and not mine (doesn't show up since I didn't edit that
method). I kept it as part of the structure since I wanted to make sure the
algorithm behaves in the same way. So might be best to address it in a separate
bug.
{quote}
{code}
/**
* Add a replica's meta information into the map
*
* @param bpid block pool id
* @param replicaInfo a replica's meta information
- * @return previous meta information of the replica
+ * @return true if inserted into the set
* @throws IllegalArgumentException if the input parameter is null
*/
- ReplicaInfo add(String bpid, ReplicaInfo replicaInfo) {
+ boolean add(String bpid, ReplicaInfo replicaInfo) {
{code}
I would like to see some clear comments in this function on what happens if
there is already a copy of the replicaInfo in the ReplicaMap. I might be wrong,
but based on my reading of TreeSet.java, it seems like the new entry won't be
added, which is a behavior change from what we did earlier. Unless I'm missing
something, this doesn't seem quite right since the new ReplicaInfo might have a
different genstamp, etc.
{quote}
Yes this is a change in behavior compared to earlier. Started down this path
since add on a Set doesn't replace, which unfortunately doesn't match what the
Map API does. I added a "replace" method in the class to be used when a replace
behavior is needed and went through the code to ensure the right method is
called when needed. Not really happy about this choice, perhaps a cleaner way
would be to have a addWithReplace method on the TreeSet and keep the old add
behavior of the ReplicaMap. I believe it would reduce the size of the patch and
only add one "ugly" method on the TreeSet.
> Improve performance and GC friendliness of startup and FBRs
> -----------------------------------------------------------
>
> Key: HDFS-9260
> URL: https://issues.apache.org/jira/browse/HDFS-9260
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: datanode, namenode, performance
> Affects Versions: 2.7.1
> Reporter: Staffan Friberg
> Assignee: Staffan Friberg
> Attachments: FBR processing.png, HDFS Block and Replica Management
> 20151013.pdf, HDFS-7435.001.patch, HDFS-7435.002.patch, HDFS-7435.003.patch,
> HDFS-7435.004.patch, HDFS-7435.005.patch, HDFS-7435.006.patch,
> HDFS-7435.007.patch, HDFS-9260.008.patch, HDFS-9260.009.patch,
> HDFS-9260.010.patch, HDFSBenchmarks.zip, HDFSBenchmarks2.zip
>
>
> This patch changes the datastructures used for BlockInfos and Replicas to
> keep them sorted. This allows faster and more GC friendly handling of full
> block reports.
> Would like to hear peoples feedback on this change.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
