hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-5326) add modifyDirective to cacheAdmin
Date Thu, 07 Nov 2013 01:28:17 GMT

    [ 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)

Mime
View raw message