[ https://issues.apache.org/jira/browse/HBASE-451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13043122#comment-13043122 ]
jirapos...@reviews.apache.org commented on HBASE-451: ----------------------------------------------------- ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/849/#review749 ----------------------------------------------------------- This looks excellent. Lets get it committed before it rots. Do we still need .regioninfo after this goes in? src/main/java/org/apache/hadoop/hbase/HConstants.java <https://reviews.apache.org/r/849/#comment1541> Copy/paste error. src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1542> WhooHoo... this is starting out good! I think you should take the opportunity of changing HRI to be a VersionedWritable. src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1543> Excellent! (HRI not having ref to HTD) src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1544> This works? Thats interesting. src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1545> When would this be used? src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1546> This should not be allowed? Are you not going to mess up stuff if table gets changed on an HRI? src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1547> Should these be deprecated rather than removed? Even if we returned a null only? src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1548> This is perverse, begin able to change the HTD under an HRI. Can't believe this actually was allowed exist. src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1549> You could just do an equals since its boolean... Bytes.equals... src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1550> ditto src/main/java/org/apache/hadoop/hbase/HRegionInfo.java <https://reviews.apache.org/r/849/#comment1551> This is incorrect. Should be isMetaTable() or isRootRegion... a 'meta' region is a .META. or -ROOT- member. src/main/java/org/apache/hadoop/hbase/HRegionInfoForMigration.java <https://reviews.apache.org/r/849/#comment1552> Put this into a migration subpackage? src/main/java/org/apache/hadoop/hbase/HRegionInfoForMigration.java <https://reviews.apache.org/r/849/#comment1554> Oh, so what is this? Its old-school HRI? If so, call it HRI090? Or if you have it in a subpackage migration, it can have same name and just be fully specified whereever it is used.. the migration subpackage will make it explicity taht this is not same as new HRI. src/main/java/org/apache/hadoop/hbase/HRegionInfoForMigration.java <https://reviews.apache.org/r/849/#comment1553> This is from elsewhere? Does it belong here? src/main/java/org/apache/hadoop/hbase/KeyValue.java <https://reviews.apache.org/r/849/#comment1555> I don't think we should do this by default. What if KV has a value of MBs? src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java <https://reviews.apache.org/r/849/#comment1557> What if we crash half-way through migration? We should be able to deal w/ a .META. that is half the old style HRI and half the new style? src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java <https://reviews.apache.org/r/849/#comment1556> You want to leave these in place? src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java <https://reviews.apache.org/r/849/#comment1558> What a crazy way of getting HTDs. src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java <https://reviews.apache.org/r/849/#comment1559> Do we need these last two? Can't we use this first method to get the latter? src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java <https://reviews.apache.org/r/849/#comment1560> Is this going to the FS to read tables? And we're holding sync while we are doing it? Is that necessary? src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java <https://reviews.apache.org/r/849/#comment1561> This method belongs in master since its going to run the migration? src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java <https://reviews.apache.org/r/849/#comment1562> Close the file? src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java <https://reviews.apache.org/r/849/#comment1563> Good. The human readable version is included. src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java <https://reviews.apache.org/r/849/#comment1564> I wonder if you have to create the file elsewhere and then do a rename to put it in place? Else if crash, could be half-written? Do we do this w/ .regioninfo? - Michael On 2011-06-02 20:52:15, Michael Stack wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/849/ bq. ----------------------------------------------------------- bq. bq. (Updated 2011-06-02 20:52:15) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. ------- bq. bq. Posting for Subbu See issue for his description of change. bq. bq. bq. This addresses bug HBASE-451. bq. https://issues.apache.org/jira/browse/HBASE-451 bq. bq. bq. Diffs bq. ----- bq. bq. src/main/java/org/apache/hadoop/hbase/HConstants.java bd4c64c bq. src/main/java/org/apache/hadoop/hbase/HRegionInfo.java 9502b1d bq. src/main/java/org/apache/hadoop/hbase/HRegionInfoForMigration.java PRE-CREATION bq. src/main/java/org/apache/hadoop/hbase/KeyValue.java 7033800 bq. src/main/java/org/apache/hadoop/hbase/catalog/MetaEditor.java a25b0f0 bq. src/main/java/org/apache/hadoop/hbase/catalog/MetaReader.java eb57197 bq. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java 7594822 bq. src/main/java/org/apache/hadoop/hbase/client/HConnection.java f722155 bq. src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java 94ee1a0 bq. src/main/java/org/apache/hadoop/hbase/client/HTable.java c82e1dd bq. src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 2734f30 bq. src/main/java/org/apache/hadoop/hbase/client/UnmodifyableHRegionInfo.java 23e7a6b bq. src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java d531b8d bq. src/main/java/org/apache/hadoop/hbase/ipc/HMasterInterface.java 4704c39 bq. src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java 38914a8 bq. src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 5b4a4b7 bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java b8489ac bq. src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java b22a3e4 bq. src/main/java/org/apache/hadoop/hbase/master/handler/ClosedRegionHandler.java c98ed17 bq. src/main/java/org/apache/hadoop/hbase/master/handler/DeleteTableHandler.java 5a8bb20 bq. src/main/java/org/apache/hadoop/hbase/master/handler/ModifyTableHandler.java 6380520 bq. src/main/java/org/apache/hadoop/hbase/master/handler/OpenedRegionHandler.java 3d16e47 bq. src/main/java/org/apache/hadoop/hbase/master/handler/ServerShutdownHandler.java dace150 bq. src/main/java/org/apache/hadoop/hbase/master/handler/TableAddFamilyHandler.java fcea483 bq. src/main/java/org/apache/hadoop/hbase/master/handler/TableDeleteFamilyHandler.java a963c6c bq. src/main/java/org/apache/hadoop/hbase/master/handler/TableModifyFamilyHandler.java 4029893 bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java e5bd154 bq. src/main/java/org/apache/hadoop/hbase/regionserver/LogRoller.java 9ccf248 bq. src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java 072fd8d bq. src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 21468ad bq. src/main/java/org/apache/hadoop/hbase/regionserver/handler/OpenRegionHandler.java ba2daa9 bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 0716788 bq. src/main/java/org/apache/hadoop/hbase/regionserver/wal/WALObserver.java 3def4b6 bq. src/main/java/org/apache/hadoop/hbase/replication/regionserver/Replication.java 1a87947 bq. src/main/java/org/apache/hadoop/hbase/util/FSUtils.java 3409108 bq. src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 39591a0 bq. src/main/java/org/apache/hadoop/hbase/util/HMerge.java c07f8dc bq. src/main/java/org/apache/hadoop/hbase/util/MetaUtils.java 540d7df bq. src/main/java/org/apache/hadoop/hbase/util/RegionSplitter.java 6d0c803 bq. src/main/java/org/apache/hadoop/hbase/util/Writables.java 3e60f97 bq. src/test/java/org/apache/hadoop/hbase/HBaseTestCase.java 4c58791 bq. src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java babd788 bq. src/test/java/org/apache/hadoop/hbase/TestCompare.java bbac815 bq. src/test/java/org/apache/hadoop/hbase/TestScanMultipleVersions.java 1f51703 bq. src/test/java/org/apache/hadoop/hbase/TestSerialization.java 05f0efc bq. src/test/java/org/apache/hadoop/hbase/TimestampTestBase.java 1105509 bq. src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java c3b23fe bq. src/test/java/org/apache/hadoop/hbase/client/TestMetaMigration.java PRE-CREATION bq. src/test/java/org/apache/hadoop/hbase/client/TestTimestamp.java db42192 bq. src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorInterface.java f0418d1 bq. src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java 46ba184 bq. src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverStacking.java 18380c6 bq. src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java 19397fb bq. src/test/java/org/apache/hadoop/hbase/filter/TestColumnPrefixFilter.java e1eb02a bq. src/test/java/org/apache/hadoop/hbase/filter/TestDependentColumnFilter.java 04705c3 bq. src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java bfa3c72 bq. src/test/java/org/apache/hadoop/hbase/master/TestCatalogJanitor.java ada2af6 bq. src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java ba87bc0 bq. src/test/java/org/apache/hadoop/hbase/master/TestLoadBalancer.java d909997 bq. src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 2022767 bq. src/test/java/org/apache/hadoop/hbase/master/TestMasterStatusServlet.java 1fef788 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestColumnSeeking.java e2f4507 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactSelection.java 48fa162 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestGetClosestAtOrBefore.java 3b7c7e8 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java e106b45 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionInfo.java 516139b bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java 40d352e bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestResettingCounters.java f092371 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestScanner.java ef8a4b2 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransaction.java b85b912 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java dbe5fb1 bq. src/test/java/org/apache/hadoop/hbase/regionserver/TestWideScanner.java 2cc197f bq. src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java bcf9024 bq. src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestHLog.java e2c158a bq. src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALObserver.java 5b95154 bq. src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestWALReplay.java 4de5b95 bq. src/test/java/org/apache/hadoop/hbase/replication/regionserver/TestReplicationSourceManager.java 3f9302a bq. src/test/java/org/apache/hadoop/hbase/rest/model/TestTableRegionModel.java c02dfda bq. src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java a6bdbe0 bq. src/test/java/org/apache/hadoop/hbase/util/TestMergeTable.java 3039df2 bq. src/test/java/org/apache/hadoop/hbase/util/TestMergeTool.java 18cd055 bq. bq. Diff: https://reviews.apache.org/r/849/diff bq. bq. bq. Testing bq. ------- bq. bq. bq. Thanks, bq. bq. Michael bq. bq. > Remove HTableDescriptor from HRegionInfo > ---------------------------------------- > > Key: HBASE-451 > URL: https://issues.apache.org/jira/browse/HBASE-451 > Project: HBase > Issue Type: Improvement > Components: master, regionserver > Affects Versions: 0.2.0 > Reporter: Jim Kellerman > Assignee: Subbu M Iyer > Priority: Critical > Fix For: 0.92.0 > > Attachments: 451_support_for_removing_HTD_from_HRI_trunk.txt, > HBASE-451_-_First_draft_support_for_removing_HTD_from_HRI1.patch > > > There is an HRegionInfo for every region in HBase. Currently HRegionInfo also > contains the HTableDescriptor (the schema). That means we store the schema n > times where n is the number of regions in the table. > Additionally, for every region of the same table that the region server has > open, there is a copy of the schema. Thus it is stored in memory once for > each open region. > If HRegionInfo merely contained the table name the HTableDescriptor could be > stored in a separate file and easily found. -- This message is automatically generated by JIRA. For more information on JIRA, see: http://www.atlassian.com/software/jira