[
https://issues.apache.org/jira/browse/HDFS-5163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13759413#comment-13759413
]
Andrew Wang commented on HDFS-5163:
-----------------------------------
Some more fixes I'd like to see:
CachePool and CachePoolInfo
* Javadoc CachePool vs. CachePoolInfo. Since the internal/external split seems
important, please document the intention. Also mention how they're used and
important fields.
* Why was the builder for CachePoolInfo removed?
* Why were equals and hashCode on CachePoolInfo removed? As a user-facing
class, having comparison functions seems useful.
* Put DEFAULT_MODE into FsPermission as {{#getCachePoolDefault()}} if you
really want 00755, I think it's also okay to just use {{#getDirDefault}} (I see
pools as kind of like folders, directives as files).
* 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.
ClientProtocol:
* I think the javadoc for the PathCacheDirective methods read better before.
Only the cache pool ID-related javadoc needs to be changed.
* removePathCacheEntries is arguably idempotent since it operates on IDs, but I
guess strictly speaking an undeletable entry could become deletable between
retries. This is a good catch.
* modifyCachePool is not idempotent, since the retry will squish any changes
that happened in the intervening time. Same permission critique applies too.
* addCachePool's javadoc now says "Modify a cache pool"
* ATM's earlier comment about removing the max # of listings parameter for both
the entries and pools APIs. Your response:
bq. It's hard to unit test unless we can explicitly set the number of entries
to return to a small number. Also, counting the total number of entries in the
map could be time-consuming (since we filter by pool, etc)
Can we make unit testing easier by having a {{VisibleForTesting}} setter for
this parameter 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.
CacheManager:
* don't need to do {{pc.isSuperUser}} since that's checked already in
{{pc.checkPermission}}
* don't we need permission checks in listPathCacheEntries and listCachePools?
FSNamesystem:
* let's not modify deleteSnapshot, if it's missing that line, let's file a
separate bug for trunk
* let's keep the {{SuppressWarnings}} annotations since IIRC they're needed for
the RetryCache.
* do we need audit logging on listCachePools?
* For consistency with other FSNamesystem methods, we should do the safemode
check before permission check everywhere.
* Sorry for missing this before, but we should also only be doing permission
checks if {{FSNamesystem#isPermissionEnabled}} is true.
* Should use {{FSNamesystem#checkSuperuserPrivilege}} for consistent exception
messages.
protobuf:
* Can we bring back the CachePoolInfoProto type? I think it made the
specification and packing/unpacking simpler. Add/Modify/List have all the same
fields right now, and we can evolve as long as the fields are optional, and can
always add more per-proto fields.
* Regardless should push the PB packing/unpacking into {{PBHelper.java}}.
* As a general statement, I'm against required fields (e.g.
ListCachePoolsResponseElementProto) unless we're really, really, really sure we
want them. Some people argue against required entirely and just do all the
validation during packing/unpacking.
> 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
