[jira] Created: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[jira] Created: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
Extract InnerClasses from FSNamesystem into separate classes
------------------------------------------------------------

                 Key: HADOOP-1272
                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
             Project: Hadoop
          Issue Type: Bug
          Components: dfs
            Reporter: dhruba borthakur
         Assigned To: dhruba borthakur


This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment: innerclasses.patch

Extracts SafeModeInfo, UnderReplciatedBlocks and FileUnderConstruction into separate files.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490261 ]

Tom White commented on HADOOP-1272:
-----------------------------------

This will also make the code clearer by breaking up the huge class into smaller chunks (FSNamesystem is over 4000 lines long!). The top ten:

find src/java/org/apache/hadoop -name *.java | xargs wc -l | sort -nr | head
  68614 total
   4277 src/java/org/apache/hadoop/dfs/FSNamesystem.java
   2682 src/java/org/apache/hadoop/io/SequenceFile.java
   1960 src/java/org/apache/hadoop/mapred/TaskTracker.java
   1717 src/java/org/apache/hadoop/mapred/JobTracker.java
   1440 src/java/org/apache/hadoop/dfs/DFSClient.java
   1309 src/java/org/apache/hadoop/dfs/DataNode.java
   1208 src/java/org/apache/hadoop/mapred/ReduceTask.java
   1095 src/java/org/apache/hadoop/fs/FsShell.java
    964 src/java/org/apache/hadoop/mapred/JobInProgress.java

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490286 ]

Andrzej Bialecki  commented on HADOOP-1272:
-------------------------------------------

Some renaming will be necessary, though, to avoid meaningless and conflicting names - e.g. SequenceFile.Reader -> SequenceFileReader, MapFile.Reader -> MapFileReader. Other inner classes with unique names could be extracted as is. There are some cases of doubly nested classes, e.g. FSNamesystem.ReplicationTargetChooser.NotEnoughReplicasException, SequenceFile.Sorter.MergeQueue, etc ... In such cases I propose to extract only the first level, to avoid exploding the parent package namespace with classes that are unusable outside their current parent class.

For that matter, perhaps in more complex cases, such as SequenceFile / MapFile / ArrayFile and associated sorters, or the family of Writable classes, it would be better to create new sub-packages? This is how it's done with compression-related classes.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490467 ]

Konstantin Shvachko commented on HADOOP-1272:
---------------------------------------------

I would start extracting servlet classes first. In general Servlets should be in a different package or sub-package, e.g.
org.apache.hadoop.dfs.servlets
Then there are some straightforward static inner classes, like Host2NodesMap.
Then BlocksMap and FileUnderConstruction, which are practically static although not declared as such.
It is easy to extract ReplicationTargetChooser, just move clusterMap inside the chooser and
provide api for modification and the access.

In general we should not multiply inner classes but rather create separate ones.
Should we add that into code review guidelines?


> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490470 ]

Doug Cutting commented on HADOOP-1272:
--------------------------------------

> org.apache.hadoop.dfs.servlets

Generally packages should be based on what the code does, not how it does it.  So I wouldn't think that the servlets should be in a sub-package.  Rather, the namenode, datanode and client might be in separate packages.

> In general we should not multiply inner classes but rather create separate ones.

Inner classes are fine so long as they don't get too big.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12490485 ]

Konstantin Shvachko commented on HADOOP-1272:
---------------------------------------------

Here is the list of classes that can be easily factored out from FSNamesystem (no new packages) :
# FsckServlet
# GetImageServlet
# Host2NodesMap
# BlocksMap
# FileUnderConstruction
# ReplicationTargetChooser

Is there a consensus on this?
Do we need to change any of the names?

> Rather, the namenode, datanode and client might be in separate packages.
We are talking for a long time about it. Should we do it now?


> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491014 ]

Doug Cutting commented on HADOOP-1272:
--------------------------------------


This seems like we're cluttering the package in order to de-clutter the
file, not a great tradeoff.  I'd rather see this as a part of a package
re-structuring.  Arguably HDFS should move into a set of packages under
org.apache.hadoop.dfs, and these would go into the namenode sub-package.
  If folks agree with that as the long-term plan, then:

- How would we name these after such a restructuring?  That's probably
what we should name them now.  The above names are probably okay in that
context, as package-private classes in the namenode package.

- Is there any reason not to restructure HDFS's packages now?  I can't
think of any.  There shouldn't be any user code that calls HDFS code
directly, should there?  So I don't think back-compatibility is an issue.

Doug




> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491071 ]

dhruba borthakur commented on HADOOP-1272:
------------------------------------------

Thanks for all the comments. In the first pass, I plan on breaking up FSNamesystem into the classes that Konstantin has suggested. I agree that, this at first sight, might appear as if we are moving clutter from inside FSNamesystem into the file-names in the dfs package. I would still like to split out the Innerclasses from FSnamesystem into their own files. This helps me in implementing file-grain lock per data structure (hadoop-1269) in a much cleaner way. Once I am done with this splitting, then I will open another JIRA issue to reorder files into packages.

Doug: please let me know if this sounds reasonable.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment:     (was: innerclasses.patch)

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment: innerclasses2.patch

Since this patch moves huge amounts of code from one file to another, the earlier patch needed lots of merging so that it is trunk-compatible.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses2.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491181 ]

Tom White commented on HADOOP-1272:
-----------------------------------

bq. Arguably HDFS should move into a set of packages under org.apache.hadoop.dfs

Or under org.apache.hadoop.fs.dfs, like the S3 code.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses2.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491358 ]

Doug Cutting commented on HADOOP-1272:
--------------------------------------

bq. Or under org.apache.hadoop.fs.dfs, like the S3 code.

Oops.  That's what I meant.  Thanks for clarifying!

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses2.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491362 ]

Doug Cutting commented on HADOOP-1272:
--------------------------------------

bq. Once I am done with this splitting, then I will open another JIRA issue to reorder files into packages.

Why not open that issue now, linking it to this?

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses2.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491456 ]

Konstantin Shvachko commented on HADOOP-1272:
---------------------------------------------

- removeNeededReplications
should take 2 more parameters rather than recalculating them
does not need to be synchronized

- updateNeededReplications
should take new and old replications rather than deltas
should not recalculate replication if it is already known

- I don't think there is a need for removeNeededReplications and updateNeededReplications
One can just use neededReplications.remove and neededReplications.update
the way it used to be.

- UnderReplicatedBlocks:
3 import warnings

- SafeModeInfo
2 import warnings
I really didn't like how the code got more complicated, because you need to startSafeModeMonitor()
in 3 places rather than one, and stateChangeLog() is printing only in one place instead of 3.
I'd just keep SafeModeInfo as an inner class.

- FileUnderConstruction
1 import warning

- Import warnings all new files.

- Why GetImageServlet and FsckServlet are not separated?
I thought nobody argued about keeping them inner as long as they are in the same package.


> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses2.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment:     (was: innerclasses2.patch)

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment: innerclasses3.patch

This incorporates most of Konstantin's comments.

1. I kept the wrapper method updateNeededReplications  but removed removeNeededReplications and addNeededReplications.

2. Removed all import warnings.

3. I kept a seperate class for SafeModeInfo because I think this is the right way to go in the long run. It will help us in getting fine-grain locking easy to understand.

4. Separated out the servlet classes into its own file(s).

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses3.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Commented: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12491498 ]

Konstantin Shvachko commented on HADOOP-1272:
---------------------------------------------

I looked more at the SafeModeInfo separation project.

# Separation is achieved by introducing code replication, which is rather dangerous,
since one can forget to include the replicated part in one of the places it should be.
Especial in case of SafeMode where miscalculation of blocks or setting a member value
can lead to loosing data blocks.
Example:
Before your patch SafeModeInfo.leave() would set safeMode = null;
Now you cannot do the same inside leave() so you should set it to null whenever leave()
is called in 3 places, namely in
leaveSafeMode(), checkMode(), and in SafeModeMonitor.run()
You did it in 2 places out of 3.
Same thing with reporting of Network topology and UnderReplicatedBlocks
NameNode.stateChangeLog.info(.......
Your patch does not report those when you leave safe mode manually.

# You completely removed the assert, which checks whether the block counting isConsistent()
because it uses FSNamesystem members.
This is the only way to check whether the block counting is right, so now our tests will not catch it.
I've seen it actually working and catching wrong doings :)

# SafeMode is not on the critical path for fine grain locking imo.
It does not have big maps or long methods, and it works mainly during cluster startup.

# JavaDoc @links and @see tags need to be updated.

I think we should keep SafeModeInfo as an inner class for the time being.
I am not saying it does not make sense to separate it, just that it is not as straightforward as other classes.

Everything else looks good to me.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses3.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment:     (was: innerclasses3.patch)

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses4.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply | Threaded
Open this post in threaded view
|

[jira] Updated: (HADOOP-1272) Extract InnerClasses from FSNamesystem into separate classes

JIRA jira@apache.org
In reply to this post by JIRA jira@apache.org

     [ https://issues.apache.org/jira/browse/HADOOP-1272?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

dhruba borthakur updated HADOOP-1272:
-------------------------------------

    Attachment: innerclasses4.patch

Hi Konstantin, thanks for the feedback. I agree that moving the SafeModeInfo out of the FSNamesystem file makes the code layout has become a little more involved that earlier. But the assumed simplicity that was earlier present was because of global variables (global with respect to FSNamesystem). I think this patch actually ensures that methods in SafeModeInfo cannot access private members of FSNamesystem directly. This should improve code maintainability and cleanliness.

The fact the SafeModeInfo cannot access private members of FSNamesystem means that a log message that was earlier present in one place is now duplicated at two or three places. I have removed the assert because SafeModeInfo cannot be accessing private variables in FSNamesystem.

Thanks for catching the problems with javadoc. I have fixed them in this version of this patch.

I would like to make this "patch available" because that might make other people review this patch.

> Extract InnerClasses from FSNamesystem into separate classes
> ------------------------------------------------------------
>
>                 Key: HADOOP-1272
>                 URL: https://issues.apache.org/jira/browse/HADOOP-1272
>             Project: Hadoop
>          Issue Type: Bug
>          Components: dfs
>            Reporter: dhruba borthakur
>         Assigned To: dhruba borthakur
>         Attachments: innerclasses4.patch
>
>
> This will make the code cleaner. Also, it leads itself to a cleaner and easily understandable finer-grain locking model.

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

12