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