[
https://issues.apache.org/jira/browse/HADOOP-13437?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15398801#comment-15398801
]
Xiao Chen edited comment on HADOOP-13437 at 7/29/16 6:29 AM:
-------------------------------------------------------------
Thanks [~andrew.wang] for the great comments! I confess I was too obsessed with
the fact that reloading doesn't load the properties, and overlooked the 'like a
big behavior change'.
However, with more digging, I still think this is right.
- In {{Configuration}} parsing, later entries with the same name win. So in a
{{Configuration}} object, a key will only have 1 value - the latest specified
value.
- Good call on swapping. Updated the code to do it this way - previously, the
old entries for default/whitelist are preserved after reloading. I think this
is purely a bug, because 1. looking at the latest config file you'll never
figure out the actual acls in memory. 2. no way to update/remove a previously
configured entry without restart.
Added a test case for this.
- Above said, I really don't see why the {{containsKey}} check should be kept.
I looked at HADOOP-10433, HADOOP-10758 and HADOOP-11341, but don't see it
mentioned. That seems to be a redundant check for me.
bq. Is the new "continue" case important? Not sure if we should ride over a
possibly malformed ACL file.
Not at all. It's ignoring the exception, and then depending on the fact
{{aclType != null}} to continue - functionally the same. I can leave that
untouched to reduce our change scope.
Patch 3 is attached, with more thorough testing. I will do another round of
test-case thinking tomorrow, but feel free to comment on what's here.
was (Author: xiaochen):
Thanks [~andrew.wang] for the great comments! I confess I was too obsessed with
the fact that reloading doesn't load the properties, and overlooked the 'like a
big behavior change'.
However, with more digging, I still think this is right.
- In {{Configuration}} parsing, later entries with the same name win. So in a
{{Configuration}} object, a key will only have 1 value - the latest specified
value.
- Good call on swapping. Updated the code to do it this way - previously, the
old entries for default/whitelist are preserved after reloading. I think this
is purely a bug, because 1. looking at the latest config file you'll never
figure out the actual acls in memory. 2. no way to update/remove a previously
configured entry without restart.
Added a test case for this.
- Above said, I really don't see why the {{containsKey}} check should be kept.
I looked at HADOOP-10433, HADOOP-10758 and HADOOP-11341, but don't see it
mentioned. That seems to be a redundant check for me.
bq. Is the new "continue" case important? Not sure if we should ride over a
possibly malformed ACL file.
Not at all. It's ignoring the exception, and then depending on the fact
{{aclType != null}} to continue - functionally the same. I can leave that
untouched to reduce our change scope.
Patch 4 is attached, with more thorough testing. I will do another round of
test-case thinking tomorrow, but feel free to comment on what's here.
> KMS should reload whitelist and default key ACLs when hot-reloading
> -------------------------------------------------------------------
>
> Key: HADOOP-13437
> URL: https://issues.apache.org/jira/browse/HADOOP-13437
> Project: Hadoop Common
> Issue Type: Bug
> Components: kms
> Affects Versions: 2.6.0
> Reporter: Xiao Chen
> Assignee: Xiao Chen
> Attachments: HADOOP-13437.01.patch, HADOOP-13437.02.patch,
> HADOOP-13437.03.patch
>
>
> When hot-reloading, {{KMSACLs#setKeyACLs}} ignores whitelist and default key
> entries if they're present in memory.
> We should reload them, hot-reload and cold-start should not have any
> difference in behavior.
> Credit to [~dilaver] for finding this.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
