[
https://issues.apache.org/jira/browse/HDFS-5326?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13815366#comment-13815366
]
Colin Patrick McCabe commented on HDFS-5326:
--------------------------------------------
bq. IdNotFoundException: Can we say what this is used for than just EINVAL?
ok. I added a comment similar to the one in {{PathNotFoundException}}
bq. Update hdfs-default.xml too with changed config name
good call
bq. ClientProtocol still revers to "cache descriptor" in
removePathBasedCacheDescriptor
fixed
bq. 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.
I don't know. I'm kind of worried about the number of
{{listPathBasedCacheDirectives}} overloads multiplying, the way the number of
{{FileSystem#create}} overloads multiplied. It seems cleaner to have one
function that can handle any of these combinations. Filter does seem like a
better name than filterInfo, though...
bq. 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.
I think your instinct is right here. PBCDi is just too long, whatever other
merits it has. But let's talk about possible renaming on another JIRA if we
can think of something better, since this patch is already kinda big...
bq. DFS#listPBCD, DFS#addPathBasedCacheDirective javadoc needs to be updated
with new params/return values
Done. I also added "list all directives visible to us" to the Javadoc.
Directives in pools that we don't have read permission on will never be listed.
bq. DFS#removePBCD: can just say "id of" instead of "id id of"
ok ok
bq. PBCD#getId / setId off
fixed
bq. CacheManager has some lines beyond 80 chars due to the new indent
fixed
bq. 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.
good catch
bq. We should add the new modify directive to DFSTestUtil so it gets tested too
ok
bq. 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?
There sort of aren't as many commonalities as it seems. The add operation
checks that everything is set-- nothing can be null. In contrast, modify
allows everything to be null, except ID. I feel like trying to factor out
methods might make it confusing. The big things, like {{DFSUtil#isValidName}},
are already common code, so I don't feel too bad about it.
> 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)
