[
https://issues.apache.org/jira/browse/HBASE-5889?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13267955#comment-13267955
]
jirapos...@reviews.apache.org commented on HBASE-5889:
------------------------------------------------------
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. >
src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon,
line 41
bq. > <https://reviews.apache.org/r/4993/diff/1/?file=106381#file106381line41>
bq. >
bq. > Is this right after Gregory's commit last night? He hides pb
Serverload inside a ServerLoad. Perhaps have to go to pb RegionLoad still even
after his commit?
bq.
bq. Gregory Chanan wrote:
bq. There isn't currently a RegionLoad that hides pb RegionLoad in the way
you've described with ServerLoad. I actually wrote one up, but didn't include
it because there is an existing RegionLoad that is needed until the
HMasterInterface is done. I felt it made more sense to wait rather than spend
a bunch of time renaming stuff.
bq.
bq. If this patch is committed as is, let's file a JIRA and assign it to
me?
bq.
bq. Michael Stack wrote:
bq. Grand!
I rebased to trunk and got Gregory's patch.
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 195
bq. > <https://reviews.apache.org/r/4993/diff/1/?file=106382#file106382line195>
bq. >
bq. > These are just not used?
bq. >
bq. > We used have a mechanism where you could subclass regionserver and
then configure it so your subclass was started out on the cluster. You think
that still possible?
It is still possible. Now, if we want to subclass regionserver, we need to use
HConstants.REGION_SERVER_IMPL. The HRegionInterface is not there any more.
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java,
line 185
bq. > <https://reviews.apache.org/r/4993/diff/1/?file=106390#file106390line185>
bq. >
bq. > I think this kinda builder is a the right thing to have over in this
util class.
You are right. I am concerned with the size of the util class. So I'd like to
have several special util classes.
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java,
line 248
bq. > <https://reviews.apache.org/r/4993/diff/1/?file=106396#file106396line248>
bq. >
bq. > Is this new or moved code?
It is moved from RegionServer, which is originally fro HRegionServer. Sorry
for the confusion.
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java,
line 1657
bq. >
<https://reviews.apache.org/r/4993/diff/1/?file=106398#file106398line1657>
bq. >
bq. > This was no longer a good idea?
It is hard to maintain two copies of implementation, especially one is not unit
tested any more.
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. >
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java, line
688
bq. > <https://reviews.apache.org/r/4993/diff/1/?file=106410#file106410line688>
bq. >
bq. > It is unexpected going to ProtobufUtils to get online regions.
This method is used in many places. Should I put the util in HRegionServer as
a static helper method?
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. > src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java,
line 315
bq. > <https://reviews.apache.org/r/4993/diff/1/?file=106414#file106414line315>
bq. >
bq. > ditto
How about put it in HRegionServer as a static helper util?
bq. On 2012-05-03 22:57:57, Michael Stack wrote:
bq. > src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java, line
1295
bq. >
<https://reviews.apache.org/r/4993/diff/1/?file=106388#file106388line1295>
bq. >
bq. > Is it going too far adding methods like this into protobufutils?
These methods seem pretty core hbase facility, too core to be out in a util
method? You have them here because there is a bunch of pb'ing going on?
bq. >
bq. > I know I suggested that we move some of the transforms out here...
but maybe this is taking it a bit far?
bq. >
bq. > What you reckon?
openRegion is used in many test classes. That's why I have this in the util.
Should I put in HRegionServer as a static helper util?
- Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4993/#review7528
-----------------------------------------------------------
On 2012-05-03 17:27:50, Jimmy Xiang wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4993/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-05-03 17:27:50)
bq.
bq.
bq. Review request for hbase.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Removed HRegionInterface, and cleaned up the HRegionServer, moved pb code
from RegionServer back to HRegionServer.
bq.
bq. The goal is to avoid two copies of region server code to maintain, and
make it possible to avoid data type conversion in the sever side.
bq.
bq. Fixed some unit tests. Now all region server unit tests test the new pb
functions.
bq.
bq. Enhanced getServerInfo so that it returns the webui port too.
bq.
bq.
bq. This addresses bug HBASE-5889.
bq. https://issues.apache.org/jira/browse/HBASE-5889
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. conf/hbase-policy.xml e45f23c
bq.
security/src/main/java/org/apache/hadoop/hbase/security/HBasePolicyProvider.java
0c4b4cb
bq.
src/main/jamon/org/apache/hadoop/hbase/tmpl/regionserver/RSStatusTmpl.jamon
87f04f4
bq. src/main/java/org/apache/hadoop/hbase/HConstants.java a9d80a0
bq. src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java e3912c2
bq. src/main/java/org/apache/hadoop/hbase/ipc/HBaseRpcMetrics.java fc9176d
bq. src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 757f98e
bq. src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java
cd9b528
bq. src/main/java/org/apache/hadoop/hbase/master/CatalogJanitor.java 79d5fdd
bq. src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 212ee3e
bq. src/main/java/org/apache/hadoop/hbase/protobuf/RequestConverter.java
d1e0993
bq. src/main/java/org/apache/hadoop/hbase/protobuf/ResponseConverter.java
81603af
bq.
src/main/java/org/apache/hadoop/hbase/protobuf/generated/AdminProtos.java
fbf0127
bq.
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ClientProtos.java
db1333b
bq.
src/main/java/org/apache/hadoop/hbase/protobuf/generated/HBaseProtos.java
ae2094d
bq. src/main/java/org/apache/hadoop/hbase/protobuf/generated/RPCProtos.java
8b45f03
bq.
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java
827fb23
bq. src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java
96ac8bd
bq.
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionThriftServer.java
4cb070e
bq. src/main/java/org/apache/hadoop/hbase/regionserver/RegionServer.java
c2c89ea
bq. src/main/protobuf/Admin.proto 2ad6fb0
bq. src/main/protobuf/RPC.proto 105fb3f
bq. src/main/resources/hbase-default.xml f54b345
bq. src/main/resources/hbase-webapps/master/table.jsp ca7310c
bq. src/test/java/org/apache/hadoop/hbase/TestDrainingServer.java a1992c3
bq. src/test/java/org/apache/hadoop/hbase/TestGlobalMemStoreSize.java
ad77e0a
bq. src/test/java/org/apache/hadoop/hbase/TestRegionRebalancing.java 5574b7f
bq. src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java
3dfc94e
bq.
src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
42092b7
bq. src/test/java/org/apache/hadoop/hbase/client/TestAdmin.java c270e28
bq. src/test/java/org/apache/hadoop/hbase/client/TestMultiParallel.java
c36272f
bq.
src/test/java/org/apache/hadoop/hbase/coprocessor/TestMasterObserver.java
bdec3ee
bq.
src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java
7dbba66
bq.
src/test/java/org/apache/hadoop/hbase/mapreduce/TestLoadIncrementalHFilesSplitRecovery.java
3acb988
bq.
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java
eb546a5
bq. src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java
ceba5cd
bq.
src/test/java/org/apache/hadoop/hbase/master/TestMasterRestartAfterDisablingTable.java
ec08b17
bq. src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java
30c6cf1
bq.
src/test/java/org/apache/hadoop/hbase/master/TestZKBasedOpenCloseRegion.java
8c3f67e
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/TestEndToEndSplitTransaction.java
7bfe4cd
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/TestRSStatusServlet.java
ffce7e8
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerMetrics.java
aa5ca37
bq.
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java
65fa948
bq. src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java fc4a0a7
bq.
bq. Diff: https://reviews.apache.org/r/4993/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. All regular and security profile tests are green before I rebased to the
latest today.
bq.
bq.
bq. Thanks,
bq.
bq. Jimmy
bq.
bq.
> Remove HRegionInterface
> -----------------------
>
> Key: HBASE-5889
> URL: https://issues.apache.org/jira/browse/HBASE-5889
> Project: HBase
> Issue Type: Improvement
> Components: client, ipc, regionserver
> Affects Versions: 0.96.0
> Reporter: Jimmy Xiang
> Assignee: Jimmy Xiang
> Fix For: 0.96.0
>
> Attachments: hbase_5889.patch, hbase_5889_v2.patch
>
>
> As a step to move internals to PB, so as to avoid the conversion for
> performance reason, we should remove the HRegionInterface.
> Therefore region server only supports ClientProtocol and AdminProtocol.
> Later on, HRegion can work with PB messages directly.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
