Thanks a lot Wei-Chiu Chuang for the review! New patch attached with comments inline:
in ContentSummary.java, the name of setter method for snapshotLength, snapshotFileCount, snapshotDirectoryCount and snapshotSpaceConsumed should be prefixed by "set". E.g. setSnapshotLength
I agree setXXX is a better setter name. The reason in these names here is for consistency with existing setter method naming. It's a public (though evolving) API, so I'd want to keep the change minimal.
in ContentSummary#equals(), you may declare a ContentSummary object and typecast the to object to it, so as to avoid explicitly typecasting every method call. This is just a personal taste, not big deal though.
Good idea, updated.
Please update FileSystemShell.md to include the -x option for the usage of du.
Good catch! Updated.
I don't understand this code in INodeDirectory, and I wonder if it has a bug. If I understand it correctly, the counts field and snapshotCounts field of summary object will be exactly the same. On the contrary, I think you may have to declare another method similar to DirectoryWithSnapshotFeature.computeContentSummary4Snapshot, but which computes content for snapshottable subdirectories and files only.
I think current patch is correct. It's a bit difficult to read through, since (the great change) of
HDFS-4995. But the high level idea is that, ContentCounts is aggregated calculation. You're right in that the calculation in INodeDirectory#computeContentSummary would aggregate same values into counts and snapshotCounts, but that's what we want. This way, in the final calculation in FsUsage$Du#processPath we can exclude the snapshot portion from the calculation by (All - snapshotAll).
I added 1 more step in the test to create a file as well, after snapshot taken. Makes sense?