[
https://issues.apache.org/jira/browse/HDFS-5121?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13755202#comment-13755202
]
Aaron T. Myers commented on HDFS-5121:
--------------------------------------
Patch looks pretty good to me. A few small comments:
# Missing {{@throws}} annotations on some of the new entries in
{{ClientProtocol}}.
# Not sure the client should be passing max # of entries to return in
{{listPathCacheEntries}} and {{listCachePools}}. Seems like the server should
set the size of the results to return, and to determine when there are no more
entries the server can include the number of remaining entries, as is done in
{{ClientProtocol#getListing}}.
# Anywhere you take the FSNS lock in a user RPC which may need to be failover
and be retried to the other NN, you should check the operation category both
before and after taking the lock:
{code}
+ List<Fallible<PathCacheEntry>> results = null;
+ writeLock();
+ try {
+ checkOperation(OperationCategory.WRITE);
{code}
See HDFS-4591 for more info.
# The error message in the case of being in safe mode in
{{removePathCacheEntries}} says "cannot *add* path cache directive."
# I'm surprised you don't take the FSNS lock at all in
{{FSNamesystem#listPathCacheEntries}}, but even if you don't have to for some
reason it seems like you should still be checking the operation category so
that client failover works properly in this case.
# You should make failed user RPCs still log audit events indicating that they
failed, e.g.:
{code}
void setOwner(String src, String username, String group)
throws AccessControlException, FileNotFoundException, SafeModeException,
UnresolvedLinkException, IOException {
try {
setOwnerInt(src, username, group);
} catch (AccessControlException e) {
logAuditEvent(false, "setOwner", src);
throw e;
}
}
{code}
In the above example {{setOwnerInt}} would log the success audit if no
exception were thrown.
# You should also use {{FSNamesystem#isAuditEnabled}} instead of directly
calling {{auditLog.isInfoEnabled}}.
# Better to use {{GenericTestUtils#assertExceptionContains}} instead of just
ignoring the expected IOE in your tests:
{code}
+ try {
+ proto.removeCachePool(pool.getId());
+ Assert.fail("expected to get an exception when " +
+ "removing a non-existent pool.");
+ } catch (IOException ioe) {
+ }
{code}
+1 once these are addressed.
> add RPCs for creating and manipulating cache pools
> --------------------------------------------------
>
> Key: HDFS-5121
> URL: https://issues.apache.org/jira/browse/HDFS-5121
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, namenode
> Affects Versions: HDFS-4949
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Attachments: hdfs-5121-3.patch, HDFS-5121-caching.001.patch,
> HDFS-5121-caching.002.patch
>
>
> We should add RPCs for creating and manipulating cache pools.
--
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
