[ 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