[
https://issues.apache.org/jira/browse/HDFS-5163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13759526#comment-13759526
]
Colin Patrick McCabe commented on HDFS-5163:
--------------------------------------------
bq. Javadoc CachePool vs. CachePoolInfo. Since the internal/external split
seems important, please document the intention. Also mention how they're used
and important fields.
Added; also added InterfaceAnnotation.
bq. Why was the builder for CachePoolInfo removed?
It's not needed since the class is mutable. If we want to make the class
immutable, let's do that in a separate JIRA.
bq. Why were equals and hashCode on CachePoolInfo removed? As a user-facing
class, having comparison functions seems useful.
There were no actual uses of this. However, since this is part of the
user-facing API, I'll add them back in.
bq. Put DEFAULT_MODE into FsPermission as #getCachePoolDefault()
OK
bq. In CachePool(), getting the current user is expensive, so it's better to
get the UGI once if necessary, and then call getters to get username/groups.
See the current constructor.
OK
bq. I think the javadoc for the PathCacheDirective methods read better before.
Only the cache pool ID-related javadoc needs to be changed.
OK, using old javadoc where appropriate
bq. Can we make unit testing easier by having a VisibleForTesting setter for
[maxEntries] on the server side? Or making it a hidden conf parameter? I'm also
not sure why counting the total # of entries factors in differently if this is
server vs. client specified. We should stretch ourselves to remove this
parameter if possible, since it is extra bytes on the wire and extra
client-side complexity.
OK. Let's make it a server-side config option.
bq. don't need to do pc.isSuperUser since that's checked already in
pc.checkPermission
ok
bq. don't we need permission checks in listPathCacheEntries and listCachePools?
listCachePools just requires superuser, which is checked in FSNamesystem.
listPathCacheEntries should indeed have those checks... fixed.
bq. don't we need permission checks in listPathCacheEntries and listCachePools?
added
bq. let's not modify deleteSnapshot, if it's missing that line, let's file a
separate bug for trunk
Filed as https://issues.apache.org/jira/browse/HDFS-5164
bq. let's keep the SuppressWarnings annotations since IIRC they're needed for
the RetryCache.
I will add them back in, for the sake of eclipse, I guess.
bq. do we need audit logging on listCachePools?
added
bq. For consistency with other FSNamesystem methods, we should do the safemode
check before permission check everywhere.
ok
bq. Sorry for missing this before, but we should also only be doing permission
checks if FSNamesystem#isPermissionEnabled is true.
that is a good catch-- added.
bq. Should use FSNamesystem#checkSuperuserPrivilege for consistent exception
messages.
ok
bq. [protobuf comments]
I agree it's a little untidy now, but we're still figuring out what we need in
the PB. let's punt these to a separate JIRA where we can discuss it more. I
filed https://issues.apache.org/jira/browse/HDFS-5166 so we can do it there.
I also don't agree that required fields are always bad. There is always the
option of creating a new RPC if you need to get out from under a required
field. This is basically the dynamic typing versus static typing debate. :)
> miscellaneous cache pool RPC fixes
> ----------------------------------
>
> Key: HDFS-5163
> URL: https://issues.apache.org/jira/browse/HDFS-5163
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, namenode
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Priority: Minor
> Attachments: HDFS-5163-caching.001.patch
>
>
> some minor fixes-- see below.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
