[
https://issues.apache.org/jira/browse/HDFS-4103?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13537710#comment-13537710
]
Aaron T. Myers commented on HDFS-4103:
--------------------------------------
bq. 1. Where is commented code without explanation? Could you show me the line
numbers?
My mistake. I missed the TODO.
bq. 2. If the test fails, we may then enable the message and re-run the test.
Is there anything wrong?
What if the test only fails occasionally? In that case re-running the test with
the logging enabled will not be helpful. This just came up recently with an
HDFS test failure that was very difficult to debug for exactly this reason.
bq. 3. We already have HDFS-4098 for append with snapshots.
Then it would be great to mention that in the TODO.
bq. 4. I don't see why the symbol "====> <====" is better then "XXX".
The symbol's not what's important, it's the text along the lines of "Test
completed, dumping tree for debugging". You can certainly go with whatever
symbol you want.
bq. 5. Is this the same as #1?
It must be. Sorry about that.
bq. 7. The method is short and simple enough to be understood. Don't you agree?
I think we are writing code but not a book for teaching Java.
I found that method difficult to follow, and I suspect others will as well.
Hence why I suggested adding a comment.
bq. 8. It is only a unit test. The current code is good enough.
I consider unit tests to be very important pieces of code.
bq. 9. You may be right but (1) I don't think addressing it does not add any
value and (2) it was not added by the patch here.
It was added by this patch, starting on line 328:
{code}
+ INodeFile getINodeFile(Path p) throws Exception {
+ final String s = p.toString();
+ return INodeFile.valueOf(fsdir.getINode(s), s);
+ }
{code}
bq. 10. testSnapshotPathINodesAfterModification(..) is current disabled.
Just because a test is disabled doesn't mean the code shouldn't be improved.
bq. 11. How many? I only found two excluding tests.
It's true those calls were mostly in tests, but the point still remains.
bq. 13. You are right that these classes are generic but they are not yet ready
to be library code and I don't want other contributors to abuse them. Let's do
it later.
It's fine if you don't want to put them in Common, but having them in the INode
class seems odd. They're certainly not INode-specific, and they're already
being (ab)used in other classes.
bq. 15. You seems don't like non-static inner classes? Is there a reason?
Mostly because it makes it impossible to test the class in isolation, since one
cannot instantiate a non-static inner class without also instantiating the
outer class. Also because it makes it more difficult to keep track of what's in
scope. In general if there's not a really good reason to have an inner class
non-static, I think it's better to make it static.
> Support O(1) snapshot creation
> ------------------------------
>
> Key: HDFS-4103
> URL: https://issues.apache.org/jira/browse/HDFS-4103
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Affects Versions: Snapshot (HDFS-2802)
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Fix For: Snapshot (HDFS-2802)
>
> Attachments: h4103_20121129.patch, h4103_20121202b.patch,
> h4103_20121202.patch, h4103_20121209b.patch, h4103_20121209.patch,
> h4103_20121210b.patch, h4103_20121210.patch, h4103_20121211.patch,
> h4103_20121212b.patch, h4103_20121212.patch, h4103_20121213.patch,
> h4103_20121215b.patch, h4103_20121215.patch, h4103_20121216.patch,
> h4103_20121217b.patch, h4103_20121217.patch, h4103_20121218.patch,
> h4103_20121219.patch, h4103_20121220.patch
>
>
> In our first snapshot implementation, snapshot creation runs in O(N) and
> occupies O(N) memory space, where N = # files + # directories + # symlinks in
> the snapshot. The advantages of the implementation are that there is no
> additional cost for the modifications after snapshots are created, and it
> leads to a simple implementation.
> In this JIRA, we optimize snapshot creation to O(1) although it introduces
> additional cost in the modifications after snapshots are created. Note that
> the INode is given as an assumption, otherwise, there is a non-constant cost
> to find the INode.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
