Details

      Description

      To future-proof the API, it'd be better if the listEZs API returned a RemoteIterator.

      1. hdfs-6780.003.patch
        38 kB
        Andrew Wang
      2. hdfs-6780.002.patch
        39 kB
        Andrew Wang
      3. hdfs-6780.001.patch
        39 kB
        Andrew Wang

        Activity

        Hide
        Andrew Wang added a comment -

        Thanks Charles, committed this to the branch.

        Show
        Andrew Wang added a comment - Thanks Charles, committed this to the branch.
        Hide
        Charles Lamb added a comment -

        LGTM. +1

        Show
        Charles Lamb added a comment - LGTM. +1
        Hide
        Andrew Wang added a comment -

        Sorry, I should add, I left equals as is because yes, there's only one field.

        Show
        Andrew Wang added a comment - Sorry, I should add, I left equals as is because yes, there's only one field.
        Hide
        Andrew Wang added a comment -

        Thanks for reviewing, new patch attached. IntelliJ IDEA by default puts two newlines near static import sections, I changed this to one. It's arguably good style though.

        Show
        Andrew Wang added a comment - Thanks for reviewing, new patch attached. IntelliJ IDEA by default puts two newlines near static import sections, I changed this to one. It's arguably good style though.
        Hide
        Charles Lamb added a comment -

        Andrew Wang,

        This looks good and it's good to get this done now since it's an API change. I just have a bunch of minor cleanups:

        EncryptionZoneIterator.java: Class comments should read "is a remote iterator that iterates over encryption zones."

        ClientNamenodeProtocolTranslatorPB.java: Extra newline added with EncryptionZonesProtos import. Ditto right before BatchedListEntries.

        EZM.java: might as well remove the unused import for EncryptionZone while we're in the neighborhood. And the extra newline before the BatchedListEntries.

        HdfsAdmin.java: Should the javadoc give the usual disclaimer about how you may not see all of the EZs if it's a large number of them and the admin adds one or more of them during the iteration?

        EncryptionZoneWithId.java: why no EqualsBuilder? Because there's only one field?

        FSD: There are unused imports for BatchedRemoteIterator and EncryptionZone.

        hdfs-default.xml: <whine>I would prefer batch.size to num.responses since the latter makes it sounds like the total number that you'll get back (ever) vs in a batch, but that horse is already out of the barn in other places so num.responses is better.</whine>

        NameNodeRpcServer.java: import BatchedListEntries is unused.

        PBHelper.java: there's an extra newline added after the EZWIP import. Ditto where you removed the two EZProtos.*Proto imports.

        TestEncryptionZones.java: "// Create and list some zones to test listing batching" reads slightly funny. Maybe "// Create and list some zones to test listEZ batching"

        Show
        Charles Lamb added a comment - Andrew Wang , This looks good and it's good to get this done now since it's an API change. I just have a bunch of minor cleanups: EncryptionZoneIterator.java: Class comments should read "is a remote iterator that iterates over encryption zones." ClientNamenodeProtocolTranslatorPB.java: Extra newline added with EncryptionZonesProtos import. Ditto right before BatchedListEntries. EZM.java: might as well remove the unused import for EncryptionZone while we're in the neighborhood. And the extra newline before the BatchedListEntries. HdfsAdmin.java: Should the javadoc give the usual disclaimer about how you may not see all of the EZs if it's a large number of them and the admin adds one or more of them during the iteration? EncryptionZoneWithId.java: why no EqualsBuilder? Because there's only one field? FSD: There are unused imports for BatchedRemoteIterator and EncryptionZone. hdfs-default.xml: <whine>I would prefer batch.size to num.responses since the latter makes it sounds like the total number that you'll get back (ever) vs in a batch, but that horse is already out of the barn in other places so num.responses is better.</whine> NameNodeRpcServer.java: import BatchedListEntries is unused. PBHelper.java: there's an extra newline added after the EZWIP import. Ditto where you removed the two EZProtos.*Proto imports. TestEncryptionZones.java: "// Create and list some zones to test listing batching" reads slightly funny. Maybe "// Create and list some zones to test listEZ batching"
        Hide
        Andrew Wang added a comment -

        Fix a compile error

        Show
        Andrew Wang added a comment - Fix a compile error
        Hide
        Andrew Wang added a comment -

        Patch attached. Refactored listEncryptionZones to instead return a RemoteIterator<EncryptionZone>, which wraps a BatchedRemoteIterator<EncryptionZoneWithId>.

        Show
        Andrew Wang added a comment - Patch attached. Refactored listEncryptionZones to instead return a RemoteIterator<EncryptionZone>, which wraps a BatchedRemoteIterator<EncryptionZoneWithId>.
        Hide
        Charles Lamb added a comment -

        Yes, hide it behind some class, like listCachePools.

        Show
        Charles Lamb added a comment - Yes, hide it behind some class, like listCachePools.
        Hide
        Yi Liu added a comment -

        I think maybe the inode ID is enough.

        Show
        Yi Liu added a comment - I think maybe the inode ID is enough.
        Hide
        Andrew Wang added a comment -

        I looked into this a bit. We need something to key the iterator. The simplest idea is to add the inode ID to EncryptionZone. We could go one step further and hide it from the end user by having yet-another-EZ-class. We could also introduce a new EZ-specific ID, which could again be hidden or exposed, and persisted or not.

        Any preference on these approaches? Colin Patrick McCabe Charles Lamb? I'm thinking of doing the hidden inode ID.

        Show
        Andrew Wang added a comment - I looked into this a bit. We need something to key the iterator. The simplest idea is to add the inode ID to EncryptionZone. We could go one step further and hide it from the end user by having yet-another-EZ-class. We could also introduce a new EZ-specific ID, which could again be hidden or exposed, and persisted or not. Any preference on these approaches? Colin Patrick McCabe Charles Lamb ? I'm thinking of doing the hidden inode ID.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development