Details

      Description

      Now that we have the basic pieces in place for encryption, it's a good time to look at our test coverage and add new tests.

      1. hdfs-6692.003.patch
        10 kB
        Andrew Wang
      2. hdfs-6692.002.patch
        9 kB
        Andrew Wang
      3. hdfs-6692.001.patch
        8 kB
        Andrew Wang

        Activity

        Hide
        Andrew Wang added a comment -

        Patch attached. I added a fault injector to do various bad things while the lock is dropped in startFile.

        Show
        Andrew Wang added a comment - Patch attached. I added a fault injector to do various bad things while the lock is dropped in startFile.
        Hide
        Andrew Wang added a comment -

        If anyone has more ideas for tests to add, feel free to pile on.

        Show
        Andrew Wang added a comment - If anyone has more ideas for tests to add, feel free to pile on.
        Hide
        Stephen Chu added a comment -

        Hi Andrew, nice tests.

        Nits:
        Can remove the concatenation in the following? "Too many retries because of " + "encryption zone operations"
        Line with the following comment goes over 80 char: * Tests the retry logic in startFile. We release the lock while generating an

        One additional test that can be added is attempting to create an encryption zone on a parent of an encryption zone.

        A minor additional test is checking that HdfsAdmin#listEncryptionZones succeeds / throws reasonable exception in testCreateEZWithNoProvider.

        I can add these tests later, too, if you prefer.

        Show
        Stephen Chu added a comment - Hi Andrew, nice tests. Nits: Can remove the concatenation in the following? "Too many retries because of " + "encryption zone operations" Line with the following comment goes over 80 char: * Tests the retry logic in startFile. We release the lock while generating an One additional test that can be added is attempting to create an encryption zone on a parent of an encryption zone. A minor additional test is checking that HdfsAdmin#listEncryptionZones succeeds / throws reasonable exception in testCreateEZWithNoProvider . I can add these tests later, too, if you prefer.
        Hide
        Andrew Wang added a comment -

        Good ideas, new patch attached. I had my right margin set at 80 chars in IDEA, but somehow it's at 81. Strange.

        Show
        Andrew Wang added a comment - Good ideas, new patch attached. I had my right margin set at 80 chars in IDEA, but somehow it's at 81. Strange.
        Hide
        Charles Lamb added a comment -

        This is a nice piece of work. My comments are stylistic.

        • Extra newline at the end of MyInjector, probably IDE induced.
        • Why do you access EFI.instance directly? Doesn't FB whine at you about having a public instance variable that you're accessing directly, or do we disable that check?
        • MyInjector is always init'd with 1,1. Perhaps just make a no-args ctor instead?
        • Naming. EncryptionFaultInjector is a relatively general name, and startFileAfterGenerateKey is more specific. But I wonder if this should be turned into a more general facility, including naming. I'm thinking something like:

        TestFaultInjector or DebugFaultInjector instead of EncryptionFaultInjector. And then perhaps startFileAfterGenerateKey could be renamed to injectFault or just inject. generateCount is more like an injectCount or injectionCount or callCount or just count.

        • If you drink that glass of Kool-Aid and agree that things could be more generally named, then you'll perhaps buy into making this a slightly more general, reusable facility (DFSTestUtil). There's repeated code:
            injector = new MyInjector(1, 1);
            EncryptionFaultInjector.instance = injector;
            future = executor.submit(new CreateFileTask(fsWrapper, file));
            injector.ready.await();
        

        and

            injector.wait.countDown();
            future.get();
            assertEquals("Expected a startFile retry", 2, injector.generateCount);
        

        So maybe the creamy filling in between those two blocks (e.g.):

            fsWrapper.delete(zone1, true);
            fsWrapper.mkdir(zone1, FsPermission.getDirDefault(), true);
            dfsAdmin.createEncryptionZone(zone1, otherKey);
        

        could be turned into a Callable?

        Of course, the fly in the ointment is the last test ("Flip-flop between two EZs to repeatedly fail") which doesn't exactly follow this pattern, so maybe this is unattainable. I'm happy with this being a follow-on Jira if you think it's a worthwhile endeavour.

        Show
        Charles Lamb added a comment - This is a nice piece of work. My comments are stylistic. Extra newline at the end of MyInjector, probably IDE induced. Why do you access EFI.instance directly? Doesn't FB whine at you about having a public instance variable that you're accessing directly, or do we disable that check? MyInjector is always init'd with 1,1. Perhaps just make a no-args ctor instead? Naming. EncryptionFaultInjector is a relatively general name, and startFileAfterGenerateKey is more specific. But I wonder if this should be turned into a more general facility, including naming. I'm thinking something like: TestFaultInjector or DebugFaultInjector instead of EncryptionFaultInjector. And then perhaps startFileAfterGenerateKey could be renamed to injectFault or just inject. generateCount is more like an injectCount or injectionCount or callCount or just count. If you drink that glass of Kool-Aid and agree that things could be more generally named, then you'll perhaps buy into making this a slightly more general, reusable facility (DFSTestUtil). There's repeated code: injector = new MyInjector(1, 1); EncryptionFaultInjector.instance = injector; future = executor.submit( new CreateFileTask(fsWrapper, file)); injector.ready.await(); and injector.wait.countDown(); future .get(); assertEquals( "Expected a startFile retry" , 2, injector.generateCount); So maybe the creamy filling in between those two blocks (e.g.): fsWrapper.delete(zone1, true ); fsWrapper.mkdir(zone1, FsPermission.getDirDefault(), true ); dfsAdmin.createEncryptionZone(zone1, otherKey); could be turned into a Callable? Of course, the fly in the ointment is the last test ("Flip-flop between two EZs to repeatedly fail") which doesn't exactly follow this pattern, so maybe this is unattainable. I'm happy with this being a follow-on Jira if you think it's a worthwhile endeavour.
        Hide
        Andrew Wang added a comment -

        The fault injector is a pattern we use elsewhere in Hadoop, you can look at CheckpointFaultInjector and co for reference. I don't know about Findbugs, but we can clear that up when we post consolidated patches for merge.

        This new patch dedupes some of the repeated injector code, but it did end up being another 40 lines because of override boilerplate. I left the last case alone because it's complicated. Tell me what you think.

        Show
        Andrew Wang added a comment - The fault injector is a pattern we use elsewhere in Hadoop, you can look at CheckpointFaultInjector and co for reference. I don't know about Findbugs, but we can clear that up when we post consolidated patches for merge. This new patch dedupes some of the repeated injector code, but it did end up being another 40 lines because of override boilerplate. I left the last case alone because it's complicated. Tell me what you think.
        Hide
        Charles Lamb added a comment -

        Nice. Thanks for remod'ing it.

        Nit: extra newline in MyInjector, but I realize that's my own little hangup so it's up to you.

        +1

        Show
        Charles Lamb added a comment - Nice. Thanks for remod'ing it. Nit: extra newline in MyInjector, but I realize that's my own little hangup so it's up to you. +1
        Hide
        Andrew Wang added a comment -

        Thanks for reviewing, I fixed the extra line at commit. Committed to branch.

        Show
        Andrew Wang added a comment - Thanks for reviewing, I fixed the extra line at commit. Committed to branch.

          People

          • Assignee:
            Andrew Wang
            Reporter:
            Andrew Wang
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development