[
https://issues.apache.org/jira/browse/HDFS-5326?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13815533#comment-13815533
]
Andrew Wang commented on HDFS-5326:
-----------------------------------
Thanks for the bump, looks almost 100%. Just a few comments:
bq. But let's talk about possible renaming on another JIRA if we can think of
something better, since this patch is already kinda big...
Sure, I'll file it.
bq. Organizationally, do you mind moving the new modify stuff in the FSEditLog,
Loader, Op, etc, so it goes add/modify/remove for directives, add/modify/remove
for pools? Compat isn't a concern yet.
This wasn't addressed, could you mind shuffling this around? I guess redoing
the opcodes is optional (though appreciated), but I'd like to see all the
methods/cases organized.
bq. There sort of aren't as many commonalities as it seems.
I took a hack at this and it ended up being less code and IMO cleaner. I can do
this in a follow-on if you like, but:
* Add and modify aren't that different besides the difference in required,
optional, and default fields. I just first validate all present fields in the
directive, then enforce required fields, then fill in default values.
* Modify and remove have the same checks for an existing entry
* Add and modify have the same checks for an existing cache pool
* All three do write checks to a cache pool, moving this into
FSPermissionChecker or a method was an easy savings
I think we should still remove the method name from the exception text
everywhere (and capitalize like a sentence). Also a few other things here:
* need to add a space
{code}
throw new IOException("addDirective: replication'" + replication +
throw new IOException("modifyDirective: replication'" + replication +
{code}
* success/fail logs are inconsistently formatted. I'd like something like e.g.
"methodName: successfully <verb> directive <directive>" and "methodName: failed
to <verb> <noun> <parameters>:", e
{code}
LOG.warn("addDirective " + directive + ": failed", e);
LOG.info("addDirective " + directive + ": succeeded.");
...
LOG.warn("modifyDirective " + idString + ": error", e);
LOG.info("modifyDirective " + idString + ": applied " + directive);
...
LOG.warn("removeDirective " + id + " failed", e);
LOG.info("removeDirective " + id + ": removed");
{code}
* I feel like we could dedupe the various PC exception texts by throwing the
AccessControlException in pc#checkPermission itself. I think it's a
straightforward change.
* Unrelated, but I noticed that CacheManager#listPBCDs does a pc check without
first checking if pc is null, want to fix that here?
* I also noticed we have some unused imports in FSEditLog and CacheManager.
> 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,
> HDFS-5326.006.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)
