[ https://issues.apache.org/jira/browse/HDFS-5326?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13815325#comment-13815325 ]
Andrew Wang commented on HDFS-5326: ----------------------------------- Hey Colin, thanks for another mondo patch. I like the overall idea here, and I like the new PB code and user-facing API. All of the comments here are minor, I'll do another pass on an updated patch but expect to quickly be +1: Nitty: * IdNotFoundException: Can we say what this is used for than just EINVAL? * Update hdfs-default.xml too with changed config name * ClientProtocol still revers to "cache descriptor" in removePathBasedCacheDescriptor * Reusing PBCD for the {{filterInfo}} in {{DFS#listPBCD}} is a little too loosey goosey for me. Normally a PBCD always has a path and pool set, with just the repl and id being optional. This is a third form of usage, and we'll probably never want to filter on more than path and pool. How about keeping the old method signature? We can still use PBCD after DFS for simplicity if you like. It should probably also be named just {{filter}} too, since the type isn't named {{PBCDInfo}}. * Maybe we should rename CachePoolInfo to CachePool so the public APIs and classes line up, e.g. you {{addPathBasedCacheDirective}} a {{PBCD}}, and {{addCachePool}} a {{CachePool}}. Or we could have a PBCDi class at the risk of shaming on Hacker News ;) If we went with PBCDi, listPBCD would of still take a {{filterInfo}}. * DFS#listPBCD, DFS#addPathBasedCacheDirective javadoc needs to be updated with new params/return values * DFS#removePBCD: can just say "id of" instead of "id id of" * PBCD#getId: javadoc says it gets the path, not the id * PBCD.Builder#setId: javadoc param descriptions are off * Organizationally, do you mind moving the new modify stuff in the FSEditLog, Loader, Op, etc, so it goes add/modify/remove for directives, add/modify/remove for pools? Compat isn't a concern yet. * CacheManager has some lines beyond 80 chars due to the new indent Other: * FSN#modifyPBCD, need to move the FSPermissionChecker get and the checkOperation above the retry cache check. We can't throw any exceptions after the retry cache check that don't also set the retry cache state. It's also I think normally first checkOperation then the pc get for consistency. * We should add the new modify directive to DFSTestUtil so it gets tested too * Seems like a lot of these checks in CacheManager are now very similar. Since there's now a try/catch wrapping everything, we no longer need to have the method name in the exception text, and it should also be in the stack trace. So, can we consolidate some of these into shared validation methods that throw generic exceptions? > add modifyDirective to cacheAdmin > --------------------------------- > > Key: HDFS-5326 > URL: https://issues.apache.org/jira/browse/HDFS-5326 > Project: Hadoop HDFS > Issue Type: Sub-task > Components: datanode, namenode > Affects Versions: 3.0.0 > Reporter: Colin Patrick McCabe > Assignee: Colin Patrick McCabe > Attachments: HDFS-5326.003.patch, HDFS-5326.004.patch > > > We should add a way of modifying cache directives on the command-line, > similar to how modifyCachePool works. -- This message was sent by Atlassian JIRA (v6.1#6144)