[
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.
