-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:https://reviews.apache.org/r/4926/
-----------------------------------------------------------

Review request for hbase and Jimmy Xiang.

Summary
-------

Convert two zk users to pb: distributed log splitting and regions in transition.

Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.

Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).

Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).

A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.

jiraposter@reviews.apache.org
added a comment - 28/Apr/12 19:10
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
Review request for hbase and Jimmy Xiang.
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 4314572
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

jiraposter@reviews.apache.org
added a comment - 28/Apr/12 23:14
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/#review7360
-----------------------------------------------------------
Looks good. Great stuff.
src/main/java/org/apache/hadoop/hbase/HBaseException.java
< https://reviews.apache.org/r/4926/#comment16239 >
That's great.
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
< https://reviews.apache.org/r/4926/#comment16240 >
Should we abort? Under what scenario the parsing can fail, other than a conflict data format?
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
< https://reviews.apache.org/r/4926/#comment16241 >
I see this many places. Can we put the whole block in a util method? Probably will be easier to read?
src/main/protobuf/ZooKeeper.proto
< https://reviews.apache.org/r/4926/#comment16242 >
A task is a path, this is more like a task state, isn't it?
Jimmy
On 2012-04-28 18:10:24, Michael Stack wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
(Updated 2012-04-28 18:10:24)
Review request for hbase and Jimmy Xiang.
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs
-----
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 4314572
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.

> Should we abort? Under what scenario the parsing can fail, other than a conflict data format?

I thought I was just redoing what was there previous. We could abort but maybe next time through the deserialization works because its been updated by another? Or, we spew this error all over the logs and drive someone crazy? Will look at it again.

jiraposter@reviews.apache.org
added a comment - 28/Apr/12 23:20
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/protobuf/ZooKeeper.proto, line 82
> < https://reviews.apache.org/r/4926/diff/1/?file=105372#file105372line82 >
>
> A task is a path, this is more like a task state, isn't it?
I can change this np.
Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java, line 798
> < https://reviews.apache.org/r/4926/diff/1/?file=105369#file105369line798 >
>
> I see this many places. Can we put the whole block in a util method? Probably will be easier to read?
Will do.
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java, line 182
> < https://reviews.apache.org/r/4926/diff/1/?file=105357#file105357line182 >
>
> Should we abort? Under what scenario the parsing can fail, other than a conflict data format?
I thought I was just redoing what was there previous. We could abort but maybe next time through the deserialization works because its been updated by another? Or, we spew this error all over the logs and drive someone crazy? Will look at it again.
Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/#review7360
-----------------------------------------------------------
On 2012-04-28 18:10:24, Michael Stack wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
(Updated 2012-04-28 18:10:24)
Review request for hbase and Jimmy Xiang.
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs
-----
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 4314572
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.

I got your point. I prefer to have the pb class named the same as the wrapper class, if there is one. Should we create a separate task state wrapper class if needed?

jiraposter@reviews.apache.org
added a comment - 28/Apr/12 23:38
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/protobuf/ZooKeeper.proto, line 82
> < https://reviews.apache.org/r/4926/diff/1/?file=105372#file105372line82 >
>
> A task is a path, this is more like a task state, isn't it?
Michael Stack wrote:
I can change this np.
Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.
I got your point. I prefer to have the pb class named the same as the wrapper class, if there is one. Should we create a separate task state wrapper class if needed?
Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/#review7360
-----------------------------------------------------------
On 2012-04-28 18:10:24, Michael Stack wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
(Updated 2012-04-28 18:10:24)
Review request for hbase and Jimmy Xiang.
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs
-----
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 4314572
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.

Jimmy Xiang wrote:

I got your point. I prefer to have the pb class named the same as the wrapper class, if there is one. Should we create a separate task state wrapper class if needed?

I just tried changing the name of this class from SplitLogTask to SplitLogTaskState and it don't seem right since you can do a 'getState' call on this class – the class has State AND the origin of the task. I'm going to leave the name as is.

Ok on keeping names the same. It should be fine if we can keep the pb stuff bottled up under the pb package or internal only to the class that uses the pb (except where pb comes out on server..)

> Should we abort? Under what scenario the parsing can fail, other than a conflict data format?

Michael Stack wrote:

I thought I was just redoing what was there previous. We could abort but maybe next time through the deserialization works because its been updated by another? Or, we spew this error all over the logs and drive someone crazy? Will look at it again.

Yeah, I'll leave this as is after looking at it. Hopefully will be good on next go around.

jiraposter@reviews.apache.org
added a comment - 28/Apr/12 23:50
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/protobuf/ZooKeeper.proto, line 82
> < https://reviews.apache.org/r/4926/diff/1/?file=105372#file105372line82 >
>
> A task is a path, this is more like a task state, isn't it?
Michael Stack wrote:
I can change this np.
Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.
Jimmy Xiang wrote:
I got your point. I prefer to have the pb class named the same as the wrapper class, if there is one. Should we create a separate task state wrapper class if needed?
I just tried changing the name of this class from SplitLogTask to SplitLogTaskState and it don't seem right since you can do a 'getState' call on this class – the class has State AND the origin of the task. I'm going to leave the name as is.
Ok on keeping names the same. It should be fine if we can keep the pb stuff bottled up under the pb package or internal only to the class that uses the pb (except where pb comes out on server..)
Thanks Jimmy
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java, line 182
> < https://reviews.apache.org/r/4926/diff/1/?file=105357#file105357line182 >
>
> Should we abort? Under what scenario the parsing can fail, other than a conflict data format?
Michael Stack wrote:
I thought I was just redoing what was there previous. We could abort but maybe next time through the deserialization works because its been updated by another? Or, we spew this error all over the logs and drive someone crazy? Will look at it again.
Yeah, I'll leave this as is after looking at it. Hopefully will be good on next go around.
Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/#review7360
-----------------------------------------------------------
On 2012-04-28 18:10:24, Michael Stack wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
(Updated 2012-04-28 18:10:24)
Review request for hbase and Jimmy Xiang.
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs
-----
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 4314572
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:https://reviews.apache.org/r/4926/
-----------------------------------------------------------

(Updated 2012-04-28 23:42:52.841896)

Review request for hbase and Jimmy Xiang.

Changes
-------

Address Jimmy's comments

Summary
-------

Convert two zk users to pb: distributed log splitting and regions in transition.

Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.

Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).

Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).

A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.

Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.

Jimmy Xiang wrote:

I got your point. I prefer to have the pb class named the same as the wrapper class, if there is one. Should we create a separate task state wrapper class if needed?

Michael Stack wrote:

I just tried changing the name of this class from SplitLogTask to SplitLogTaskState and it don't seem right since you can do a 'getState' call on this class – the class has State AND the origin of the task. I'm going to leave the name as is.

Ok on keeping names the same. It should be fine if we can keep the pb stuff bottled up under the pb package or internal only to the class that uses the pb (except where pb comes out on server..)

> Should we abort? Under what scenario the parsing can fail, other than a conflict data format?

Michael Stack wrote:

I thought I was just redoing what was there previous. We could abort but maybe next time through the deserialization works because its been updated by another? Or, we spew this error all over the logs and drive someone crazy? Will look at it again.

Michael Stack wrote:

Yeah, I'll leave this as is after looking at it. Hopefully will be good on next go around.

jiraposter@reviews.apache.org
added a comment - 30/Apr/12 18:01
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/protobuf/ZooKeeper.proto, line 82
> < https://reviews.apache.org/r/4926/diff/1/?file=105372#file105372line82 >
>
> A task is a path, this is more like a task state, isn't it?
Michael Stack wrote:
I can change this np.
Currently I have the pb class named same as the class that wraps it. Should I change this? Add a pb prefix or something? Problem w/ that is that no other of the pb classes have the pb prefix. They are in the generated package which is probably sufficient to distingush them? My hope is to make it so the pbs do not leak outside of the class that serializes to them; e.g. this SplitLogTask class.
Jimmy Xiang wrote:
I got your point. I prefer to have the pb class named the same as the wrapper class, if there is one. Should we create a separate task state wrapper class if needed?
Michael Stack wrote:
I just tried changing the name of this class from SplitLogTask to SplitLogTaskState and it don't seem right since you can do a 'getState' call on this class – the class has State AND the origin of the task. I'm going to leave the name as is.
Ok on keeping names the same. It should be fine if we can keep the pb stuff bottled up under the pb package or internal only to the class that uses the pb (except where pb comes out on server..)
Thanks Jimmy
Ok, that's fine with me.
On 2012-04-28 22:14:23, Jimmy Xiang wrote:
> src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java, line 182
> < https://reviews.apache.org/r/4926/diff/1/?file=105357#file105357line182 >
>
> Should we abort? Under what scenario the parsing can fail, other than a conflict data format?
Michael Stack wrote:
I thought I was just redoing what was there previous. We could abort but maybe next time through the deserialization works because its been updated by another? Or, we spew this error all over the logs and drive someone crazy? Will look at it again.
Michael Stack wrote:
Yeah, I'll leave this as is after looking at it. Hopefully will be good on next go around.
Ok
Jimmy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/#review7360
-----------------------------------------------------------
On 2012-04-28 23:42:52, Michael Stack wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
(Updated 2012-04-28 23:42:52)
Review request for hbase and Jimmy Xiang.
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs
-----
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 692f194
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 4314572
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

I was returning early in AssignmentManager if null data inside isCarryingRegion when I should have carried on to trip over the get of region location from the AM memory. Seems to fix some of the failing tests.

stack
added a comment - 30/Apr/12 22:12 I was returning early in AssignmentManager if null data inside isCarryingRegion when I should have carried on to trip over the get of region location from the AM memory. Seems to fix some of the failing tests.

Hadoop QA
added a comment - 01/May/12 08:19 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12525158/v10.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 47 new or modified tests.
+1 hadoop23. The patch compiles against the hadoop 0.23.x profile.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.regionserver.TestServerCustomProtocol
org.apache.hadoop.hbase.client.TestShell
org.apache.hadoop.hbase.master.TestMasterFailover
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1703//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1703//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1703//console
This message is automatically generated.

Hadoop QA
added a comment - 01/May/12 20:00 -1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12525189/v11.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 47 new or modified tests.
+1 hadoop23. The patch compiles against the hadoop 0.23.x profile.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
-1 core tests. The patch failed these unit tests:
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1707//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1707//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1707//console
This message is automatically generated.

Hadoop QA
added a comment - 01/May/12 21:27 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12525202/v12.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
+1 hadoop23. The patch compiles against the hadoop 0.23.x profile.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1710//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1710//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1710//console
This message is automatically generated.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:https://reviews.apache.org/r/4926/
-----------------------------------------------------------

(Updated 2012-05-01 20:42:36.337375)

Review request for hbase and Jimmy Xiang.

Changes
-------

Same as original w/ a few fixes for tests that failed:

1. In distributed log tests, was failing to pick up the recovered.edits file because string passed included state of the split log task when what was wanted was servername only

Summary
-------

Convert two zk users to pb: distributed log splitting and regions in transition.

Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.

Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).

Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).

A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.

jiraposter@reviews.apache.org
added a comment - 01/May/12 21:42
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4926/
-----------------------------------------------------------
(Updated 2012-05-01 20:42:36.337375)
Review request for hbase and Jimmy Xiang.
Changes
-------
Same as original w/ a few fixes for tests that failed:
1. In distributed log tests, was failing to pick up the recovered.edits file because string passed included state of the split log task when what was wanted was servername only
Summary
-------
Convert two zk users to pb: distributed log splitting and regions in transition.
Refactored distributed log splitting so we only serialize/deserialize in one location.
Less changes needed to do same for regions in transition.
Moves serialization/deserialization out of the ZKAssign, ZKSplit and into
the classes themselves so can encapsulate how serialization is done into one place
(try to make the ZK* classes just deal in bytes – about 90% done).
Moved classes used by various packages up to top level to minimize imports
that are across package (zookeeper into protobuf and/or into regionserver and/or
master packages, etc).
A src/main/java/org/apache/hadoop/hbase/DeserializationException.java
New generic deserialization exception.
A src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java
D src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java
Moved under zookeeper package.
A src/main/java/org/apache/hadoop/hbase/HBaseException.java
New base hbase exception as suggested by hbase-5796. New DeserializationException
inherits from this.
A src/main/java/org/apache/hadoop/hbase/RegionTransition.java
State of a region in transition. Top-level because used by a
few top-level packages. Encapsulates pb serialization/deserialization.
M src/main/java/org/apache/hadoop/hbase/ServerName.java
Add method to deserialize a ServeName, etc. Encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java
Counters used by distributed log splitting.
A SplitLogTask
Class that encapsulates log splitting state. Also encapsulates pb'ing.
M src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java
Implement code for state. Added functions to go from code to state and vice
versa. Used serializing.
M src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java
Remove unused imports.
D src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java
Removed. Replaced by RegionTransition moved to package top-level.
M src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java
M src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java
Use new DeserializationException. Move to using new RegionTransition
from RegionTransitionData class. Pass deserialized class rather than
byte array. Remove duplicated code.
M src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Use new ServerName parse method rather than ZKUtil one.
M src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java
Redo to use new SplitLogTask and SplitLogCounter classes.
M src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java
expectPBMagicPrefix added
M src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java
Use new RegionTransition in place of RegionTransitionData.
M src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java
Define moved from ZKSplitLog to SplitLogManager.
M src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java
M src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java
Changed method name from getZNodeData to toByteArray to match how we've
named it elsewhere. Use new DeserializationException
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java
Use new RegionTransion class
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java
Moved stuff that was in here up into SplitLogManager where better
belongs. Also moved serialization/deserialization up into the
class itself: SplitLogTask. Moved counters out to SplitLogCounter class.
M src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java
Moved deserialization of ServerName out of here and up into ServerName.
M src/main/protobuf/ZooKeeper.proto
Add two new classes, RegionTransition and SplitLogTask.
This addresses bug HBASE-5869 .
https://issues.apache.org/jira/browse/HBASE-5869
Diffs (updated)
src/main/java/org/apache/hadoop/hbase/DeserializationException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/EmptyWatcher.java 9881ec2
src/main/java/org/apache/hadoop/hbase/HBaseException.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/RegionTransition.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/ServerName.java 8fdb624
src/main/java/org/apache/hadoop/hbase/SplitLogCounters.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/SplitLogTask.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/executor/EventHandler.java 4121508
src/main/java/org/apache/hadoop/hbase/executor/ExecutorService.java 06ca377
src/main/java/org/apache/hadoop/hbase/executor/RegionTransitionData.java 35d7b70
src/main/java/org/apache/hadoop/hbase/master/ActiveMasterManager.java 47e3bd6
src/main/java/org/apache/hadoop/hbase/master/AssignmentManager.java f56127d
src/main/java/org/apache/hadoop/hbase/master/HMaster.java 81e9023
src/main/java/org/apache/hadoop/hbase/master/MasterFileSystem.java 692f194
src/main/java/org/apache/hadoop/hbase/master/SplitLogManager.java 919c65f
src/main/java/org/apache/hadoop/hbase/protobuf/ProtobufUtil.java 994cb76
src/main/java/org/apache/hadoop/hbase/protobuf/generated/ZooKeeperProtos.java 8457bdc
src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java ebffad6
src/main/java/org/apache/hadoop/hbase/regionserver/SplitLogWorker.java 8ea342f
src/main/java/org/apache/hadoop/hbase/regionserver/SplitTransaction.java ea12da4
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLog.java 914b0d3
src/main/java/org/apache/hadoop/hbase/regionserver/wal/HLogSplitter.java 587386c
src/main/java/org/apache/hadoop/hbase/zookeeper/EmptyWatcher.java PRE-CREATION
src/main/java/org/apache/hadoop/hbase/zookeeper/MasterAddressTracker.java f9575af
src/main/java/org/apache/hadoop/hbase/zookeeper/RootRegionTracker.java babde80
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKAssign.java e94b672
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKSplitLog.java 30d7fe9
src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 46a6fde
src/main/protobuf/ZooKeeper.proto 961ab65
src/test/java/org/apache/hadoop/hbase/HBaseTestingUtility.java c3a1889
src/test/java/org/apache/hadoop/hbase/TestSerialization.java 50cb9d4
src/test/java/org/apache/hadoop/hbase/TestZooKeeper.java d496d48
src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditor.java 1105ec9
src/test/java/org/apache/hadoop/hbase/coprocessor/TestWALObserver.java 36dd289
src/test/java/org/apache/hadoop/hbase/master/Mocking.java 676d6bb
src/test/java/org/apache/hadoop/hbase/master/TestAssignmentManager.java 36046f8
src/test/java/org/apache/hadoop/hbase/master/TestDistributedLogSplitting.java 2669876
src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java 14cdb90
src/test/java/org/apache/hadoop/hbase/master/TestMasterNoCluster.java f8029ba
src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java 0f7d54e
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitLogWorker.java 26b9865
src/test/java/org/apache/hadoop/hbase/regionserver/TestSplitTransactionOnCluster.java 75b5aea
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestCloseRegionHandler.java 07f8fc4
src/test/java/org/apache/hadoop/hbase/regionserver/handler/TestOpenRegionHandler.java 55a8c4a
src/test/java/org/apache/hadoop/hbase/util/TestHBaseFsck.java 01dff76
Diff: https://reviews.apache.org/r/4926/diff
Testing
-------
Thanks,
Michael

stack
added a comment - 01/May/12 21:44 What I applied to rb and what I want to commit. Same as v12 only removes needless region creation over in testwalobserver tests... was causing testwalobserver take time going down.

Hadoop QA
added a comment - 01/May/12 22:36 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12525216/v13.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
+1 hadoop23. The patch compiles against the hadoop 0.23.x profile.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1711//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1711//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1711//console
This message is automatically generated.

Ted Yu
added a comment - 02/May/12 05:42 According to https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/InvalidProtocolBufferException , InvalidProtocolBufferException doesn't provide its own method for revealing what went wrong during parsing.
In the patch, I see the following construct:
} catch (InvalidProtocolBufferException e) {
throw new DeserializationException(e);
I think we don't expect client to interpret InvalidProtocolBufferException. I would suggest changing the above to:
} catch (InvalidProtocolBufferException e) {
throw new DeserializationException(e.getMessage());

stack
added a comment - 02/May/12 16:20 I don't follow. Please give example illustrating the difference you are trying to bring out.
As is, will show IPBE as cause and if a message, that'll show as part of the cause. Yours would look to squash the exception stack to just the message... if there is one at all.

Searching through the patch, I don't see InvalidProtocolBufferException being parsed/unwrapped out of DeserializationException.
DeserializationException provides isolation to clients w.r.t. various exceptions such as InvalidProtocolBufferException.
So there is no loss not keeping the stack of InvalidProtocolBufferException.

Ted Yu
added a comment - 02/May/12 16:51 Searching through the patch, I don't see InvalidProtocolBufferException being parsed/unwrapped out of DeserializationException.
DeserializationException provides isolation to clients w.r.t. various exceptions such as InvalidProtocolBufferException.
So there is no loss not keeping the stack of InvalidProtocolBufferException.

If a DE comes out, I don't think it a good idea that we cut the stack trace off at the knees (I'm new to pb; would like to see some of these exceptions first before I start making calls on how we might massage them)

stack
added a comment - 02/May/12 17:00 If a DE comes out, I don't think it a good idea that we cut the stack trace off at the knees (I'm new to pb; would like to see some of these exceptions first before I start making calls on how we might massage them)

Hadoop QA
added a comment - 02/May/12 17:15 +1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12525301/v13.txt
against trunk revision .
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 50 new or modified tests.
+1 hadoop23. The patch compiles against the hadoop 0.23.x profile.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.
+1 release audit. The applied patch does not increase the total number of release audit warnings.
+1 core tests. The patch passed unit tests in .
Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/1723//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/1723//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/1723//console
This message is automatically generated.

Ted Yu
added a comment - 02/May/12 17:39 Let me fix toStringBinary so it deals w/ case where data is < 64 bytes.
Is the above done ?
According to http://docs.oracle.com/javase/6/docs/api/java/lang/String.html#String%28byte[],%20int,%20int,%20java.lang.String%29 :
IndexOutOfBoundsException - If the offset and length arguments index characters outside the bounds of the bytes array
> Should e1 be included in the log ?
Will add it.
Is the above done ?