[ https://issues.apache.org/jira/browse/HDFS-5119?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13785558#comment-13785558 ]
Chris Nauroth commented on HDFS-5119: ------------------------------------- Hi, Andrew. This looks good. I ran the patch in a test cluster and did a successful layout version upgrade. I ran all caching-related operations multiple times, forced a checkpoint, and restarted the namenode to confirm that it maintained the correct state. Nice job! Here are a few minor comments: # {{Text#readString(DataInput)}} is now equivalent to {{Text#readString(DataInput, int)}} passing {{Integer#MAX_VALUE}} for the second argument. Do you want to call the 2-arg method from the 1-arg method to cut some duplication? # Is it time to remove the following TODO? {code} CacheManager(FSNamesystem namesystem, FSDirectory dir, Configuration conf) { // TODO: support loading and storing of the CacheManager state {code} # Please add JavaDocs for the following {{CacheManager}} methods: {{unprotectedAddEntry}}, {{addDirective}}, {{unprotectedAddDirective}}, {{unprotectedRemoveDescriptor}}, {{unprotectedAddCachePool}}. # I noticed that {{FSNamesystem#setCacheReplicationInt}} logs an audit event for "setReplication" instead of "setCacheReplication". Do you want to change that string right now, or do you prefer if I file a new bug report? # Regarding the following code, do we need to intercept setReplication calls on existing files and also update cache replication to keep them in sync? Also, I notice this is skipping directories. Will this code change when we add support for caching a directory in HDFS-5096? {code} INode node = dir.getINode(entry.getPath()); if (node != null && node.isFile()) { INodeFile file = node.asFile(); // TODO: adjustable cache replication factor namesystem.setCacheReplicationInt(entry.getPath(), file.getBlockReplication()); } else { LOG.warn("Path " + entry.getPath() + " is not a file"); } {code} > Persist CacheManager state in the edit log > ------------------------------------------ > > Key: HDFS-5119 > URL: https://issues.apache.org/jira/browse/HDFS-5119 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: namenode > Affects Versions: HDFS-4949 > Reporter: Colin Patrick McCabe > Assignee: Andrew Wang > Attachments: hdfs-5119-1.patch, hdfs-5119-2.patch > > > CacheManager state should be persisted in the edit log. At the moment, this > state consists of information about cache pools and cache directives. It's > not necessary to persist any information about what is cached on the > DataNodes at any particular moment, since this changes all the time. -- This message was sent by Atlassian JIRA (v6.1#6144)