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.
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.