hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Colin Patrick McCabe (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (HDFS-9260) Improve performance and GC friendliness of startup and FBRs
Date Tue, 26 Jan 2016 23:30:40 GMT

    [ https://issues.apache.org/jira/browse/HDFS-9260?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15118269#comment-15118269
] 

Colin Patrick McCabe edited comment on HDFS-9260 at 1/26/16 11:30 PM:
----------------------------------------------------------------------

bq. Should I convert storages field to private? (The triplets field was protected)

That sounds like a good idea.  We have functions to manipulate these fields, so the subclasses
don't need to directly poke at the fields.

If that involves code changes to the subclasses, though, let's just do it in a follow-on JIRA.
 This is one case where checkstyle is not that useful, since it's warning about a problem
that already exists before this patch (and isn't even really a "problem," just an infelicity).

{code}
-  public static final String  DFS_NAMENODE_REPLICATION_STREAMS_HARD_LIMIT_KEY = "dfs.namenode.replication.max-streams-hard-limit";
+  public static final String  DFS_NAMENODE_REPLICATION_STREAMS_HARD_LIMIT_KEY =
+      "dfs.namenode.replication.max-streams-hard-limit";
{code}
Can we skip this change?  It's not really part of this work and it makes the diff bigger.

{code}
+  public static final String DFS_NAMENODE_STORAGEINFO_EFFICIENCY_INTERVAL_KEY
+      = "dfs.namenode.storageinfo.efficiency.interval";
+  public static final int DFS_NAMENODE_STORAGEINFO_EFFICIENCY_INTERVAL_DEFAULT
+      = 600;
{code}
Should be something like {{dfs.namenode.storageinfo.defragmenter.interval.ms}} to indicate
that it's a scan interval, and that it's in milliseconds.

{code}
+    Object[] old = storages;
+    storages = new DatanodeStorageInfo[(last+num)];
+    System.arraycopy(old, 0, storages, 0, last);
{code}
Now "old" can have type {{DatanodeStorageInfo[]}} rather than {{Object[]}}, right?

{code}
+      storageInfoMonitorThread.interrupt();
{code}
Maybe this should be something like {{storageInfoDefragmenterThread}}? "monitor" suggests
something like the block scanner, not defragmentation (at least in my mind?)

{code}
import org.apache.hadoop.hdfs.util.TreeSet;
{code}
I think this might be less confusing if you called it {{ChunkedTreeSet}}.  If I were just
a new developer looking at the code (or even an experienced developer), I wouldn't really
expect us to be using something called {{TreeSet}} which was actually completely different
than {{java.util.TreeSet}}.

{code}
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/proto/DatanodeProtocol.proto
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/proto/DatanodeProtocol.proto
@@ -246,6 +246,7 @@ message BlockReportRequestProto {
   required string blockPoolId = 2;
   repeated StorageBlockReportProto reports = 3;
   optional BlockReportContextProto context = 4;
+  optional bool sorted = 5 [default = false];
 }
{code}
The other fields in {{BlockReportRequestProto}} have comments explaining what they are.  Let's
add one for "sorted"


was (Author: cmccabe):
bq. Should I convert storages field to private? (The triplets field was protected)

That sounds like a good idea.  We have functions to manipulate these fields, so the subclasses
don't need to directly poke at the fields.

If that involves code changes to the subclasses, though, let's just do it in a follow-on JIRA.
 This is one case where checkstyle is not that useful, since it's warning about a problem
that already exists before this patch (and isn't even really a "problem," just an infelicity).

{code}
-  public static final String  DFS_NAMENODE_REPLICATION_STREAMS_HARD_LIMIT_KEY = "dfs.namenode.replication.max-streams-hard-limit";
+  public static final String  DFS_NAMENODE_REPLICATION_STREAMS_HARD_LIMIT_KEY =
+      "dfs.namenode.replication.max-streams-hard-limit";
{code}
Can we skip this change?  It's not really part of this work and it makes the diff bigger.

{code}
+  public static final String DFS_NAMENODE_STORAGEINFO_EFFICIENCY_INTERVAL_KEY
+      = "dfs.namenode.storageinfo.efficiency.interval";
+  public static final int DFS_NAMENODE_STORAGEINFO_EFFICIENCY_INTERVAL_DEFAULT
+      = 600;
{code}
Should be something like {{dfs.namenode.storageinfo.defragmenter.interval.ms}} to indicate
that it's a scan interval, and that it's in milliseconds.

{code}
-      String poolId, StorageBlockReport[] reports, BlockReportContext context)
+      String poolId, StorageBlockReport[] reports, boolean sorted,
+      BlockReportContext context)
{code}
Another unnecessary diff

{code}
+    Object[] old = storages;
+    storages = new DatanodeStorageInfo[(last+num)];
+    System.arraycopy(old, 0, storages, 0, last);
{code}
Now "old" can have type {{DatanodeStorageInfo[]}} rather than {{Object[]}}, right?

{code}
+      storageInfoMonitorThread.interrupt();
{code}
Maybe this should be something like {{storageInfoDefragmenterThread}}? "monitor" suggests
something like the block scanner, not defragmentation (at least in my mind?)

{code}
import org.apache.hadoop.hdfs.util.TreeSet;
{code}
I think this might be less confusing if you called it {{ChunkedTreeSet}}.  If I were just
a new developer looking at the code (or even an experienced developer), I wouldn't really
expect us to be using something called {{TreeSet}} which was actually completely different
than {{java.util.TreeSet}}.

{code}
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/proto/DatanodeProtocol.proto
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/proto/DatanodeProtocol.proto
@@ -246,6 +246,7 @@ message BlockReportRequestProto {
   required string blockPoolId = 2;
   repeated StorageBlockReportProto reports = 3;
   optional BlockReportContextProto context = 4;
+  optional bool sorted = 5 [default = false];
 }
{code}
The other fields in {{BlockReportRequestProto}} have comments explaining what they are.  Let's
add one for "sorted"

> 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,
HDFS-9260.011.patch, HDFS-9260.012.patch, HDFS-9260.013.patch, HDFS-9260.014.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)

Mime
View raw message