Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.2.0
    • Fix Version/s: 0.2.0
    • Component/s: None
    • Labels:
      None

      Description

      Move:

      • HRegionServer
      • HRegionInterface
      • HRegion
      • HStore
      • HStoreFile
      • HInternalScannerInterface ?
      • HStoreKey

      Others?

      1. 419.patch
        938 kB
        Bryan Duxbury

        Issue Links

          Activity

          Hide
          Jim Kellerman added a comment -

          > Should store be subpackage of regionserver rather than a peer?

          Both master.BaseScanner and util.Migrate use HStore.isReference but that is already public, so where it lives is not that important I guess.

          Show
          Jim Kellerman added a comment - > Should store be subpackage of regionserver rather than a peer? Both master.BaseScanner and util.Migrate use HStore.isReference but that is already public, so where it lives is not that important I guess.
          Hide
          stack added a comment -

          We should just do the Jim suggestions in a new issue? If you ain't doing it Bryan, give it to me and I'll do it. Good stuff.

          Show
          stack added a comment - We should just do the Jim suggestions in a new issue? If you ain't doing it Bryan, give it to me and I'll do it. Good stuff.
          Hide
          stack added a comment -

          I like jim's comments.

          +1 on idea of making a package for log (I'd suggest log rather than hlog).... .

          Should store be subpackage of regionserver rather than a peer?

          +1 on moving HRegionInterface etc. into ipc package.

          +1 on HSK being at top level rather than under regionmanager

          Show
          stack added a comment - I like jim's comments. +1 on idea of making a package for log (I'd suggest log rather than hlog).... . Should store be subpackage of regionserver rather than a peer? +1 on moving HRegionInterface etc. into ipc package. +1 on HSK being at top level rather than under regionmanager
          Hide
          Bryan Duxbury added a comment -

          I just committed this, per Stack's suggestion. (Should have checked the issue again and seen Jim's comments...) I will address javadoc issues in the new issue Jim created.

          Show
          Bryan Duxbury added a comment - I just committed this, per Stack's suggestion. (Should have checked the issue again and seen Jim's comments...) I will address javadoc issues in the new issue Jim created.
          Hide
          Jim Kellerman added a comment -

          Do the constructors for ProcessRegionXxx need to be public? Shouldn't they be package scoped?

          You didn't move HScannerInterface, Leases or HMsg because they are used in multiple places.

          Why move:

          • HRegionInterface (used by master and client)
          • HMasterInterface (used by master and client)
          • HMasterRegionInterface (used by master and region server)
          • HStoreKey (used almost everywhere)

          If I were going to move everything, I might do something like:

          • put HLog, HLogKey, HLogEdit, LogRollListener into o.a.h.h.hlog
          • put HStore, HStoreKey, HStoreFile, HStoreSize into o.a.h.h.hstore
          • put HRegionInterface, HMasterInterface and HMasterRegionInterface into o.a.h.h.ipc

          javadoc warnings: (must be fixed)

          [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\mapred\GroupingTableMap.java:91: warning - Tag @see: can't find map(org.apache.hadoop.hbase.HStoreKey, org.apache.hadoop.io.MapWritable, org.apache.hadoop.mapred.OutputCollector, org.apache.hadoop.mapred.Reporter) in org.apache.hadoop.hbase.mapred.TableMap
          [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\mapred\IdentityTableMap.java:47: warning - Tag @see: can't find map(org.apache.hadoop.hbase.HStoreKey, org.apache.hadoop.io.MapWritable, org.apache.hadoop.mapred.OutputCollector, org.apache.hadoop.mapred.Reporter) in org.apache.hadoop.hbase.mapred.TableMap
          [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\mapred\TableInputFormat.java:58: warning - Tag @see: reference not found: org.apache.hadoop.hbase.HAbstractScanner for column name wildcards
          [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\regionserver\HAbstractScanner.java:205: warning - Tag @see: can't find next(org.apache.hadoop.hbase.HStoreKey, java.util.SortedMap) in org.apache.hadoop.hbase.HScannerInterface
          [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\regionserver\HRegion.java:713: warning - @return tag has no arguments.
          [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\regionserver\HRegionServer.java:993: warning - Tag @link: reference not found: Flusher

          Show
          Jim Kellerman added a comment - Do the constructors for ProcessRegionXxx need to be public? Shouldn't they be package scoped? You didn't move HScannerInterface, Leases or HMsg because they are used in multiple places. Why move: HRegionInterface (used by master and client) HMasterInterface (used by master and client) HMasterRegionInterface (used by master and region server) HStoreKey (used almost everywhere) If I were going to move everything, I might do something like: put HLog, HLogKey, HLogEdit, LogRollListener into o.a.h.h.hlog put HStore, HStoreKey, HStoreFile, HStoreSize into o.a.h.h.hstore put HRegionInterface, HMasterInterface and HMasterRegionInterface into o.a.h.h.ipc javadoc warnings: (must be fixed) [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\mapred\GroupingTableMap.java:91: warning - Tag @see: can't find map(org.apache.hadoop.hbase.HStoreKey, org.apache.hadoop.io.MapWritable, org.apache.hadoop.mapred.OutputCollector, org.apache.hadoop.mapred.Reporter) in org.apache.hadoop.hbase.mapred.TableMap [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\mapred\IdentityTableMap.java:47: warning - Tag @see: can't find map(org.apache.hadoop.hbase.HStoreKey, org.apache.hadoop.io.MapWritable, org.apache.hadoop.mapred.OutputCollector, org.apache.hadoop.mapred.Reporter) in org.apache.hadoop.hbase.mapred.TableMap [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\mapred\TableInputFormat.java:58: warning - Tag @see: reference not found: org.apache.hadoop.hbase.HAbstractScanner for column name wildcards [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\regionserver\HAbstractScanner.java:205: warning - Tag @see: can't find next(org.apache.hadoop.hbase.HStoreKey, java.util.SortedMap) in org.apache.hadoop.hbase.HScannerInterface [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\regionserver\HRegion.java:713: warning - @return tag has no arguments. [javadoc] C:\workspace\HBase-test\src\java\org\apache\hadoop\hbase\regionserver\HRegionServer.java:993: warning - Tag @link: reference not found: Flusher
          Hide
          stack added a comment -

          Patch failed to apply to TRUNK:

          patching file src/test/org/apache/hadoop/hbase/TestScannerAPI.java
          Hunk #1 FAILED at 30.
          1 out of 1 hunk FAILED -- saving rejects to file src/test/org/apache/hadoop/hbase/TestScannerAPI.java.rej
          

          Patch mostly all class moves. Changes don't show because of class moves. I'd say for this issue, keep the moves to the maximum and minimize how much you change the actual classes. Fix the above failed patch application and just apply it.

          Show
          stack added a comment - Patch failed to apply to TRUNK: patching file src/test/org/apache/hadoop/hbase/TestScannerAPI.java Hunk #1 FAILED at 30. 1 out of 1 hunk FAILED -- saving rejects to file src/test/org/apache/hadoop/hbase/TestScannerAPI.java.rej Patch mostly all class moves. Changes don't show because of class moves. I'd say for this issue, keep the moves to the maximum and minimize how much you change the actual classes. Fix the above failed patch application and just apply it.
          Hide
          Bryan Duxbury added a comment -

          This patch moves all the classes and tests, adds needed imports all over the place, and makes some previously package-access methods public, mostly for the sake of tests. All unit tests pass locally. Please review.

          Show
          Bryan Duxbury added a comment - This patch moves all the classes and tests, adds needed imports all over the place, and makes some previously package-access methods public, mostly for the sake of tests. All unit tests pass locally. Please review.

            People

            • Assignee:
              Bryan Duxbury
              Reporter:
              Bryan Duxbury
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development