busbey commented on a change in pull request #2254:
URL: https://github.com/apache/hbase/pull/2254#discussion_r470395358
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -784,9 +791,12 @@ default void move(byte[] encodedRegionName, byte[]
destServerName) throws IOExce
* @param force If <code>true</code>, force unassign (Will remove region
from regions-in-transition too if
* present. If results in double assignment use hbck -fix to resolve. To be
used by experts).
* @throws IOException if a remote or network exception occurs
+ * @deprecated since 2.4.0 and will be removed in 4.0.0. Use {@link
#unassign(byte[])}
+ * instead.
+ * @see <a
href="https://issues.apache.org/jira/browse/HBASE-24875";>HBASE-24875</a>
*/
- void unassign(byte[] regionName, boolean force)
- throws IOException;
+ @Deprecated
+ void unassign(byte[] regionName, boolean force) throws IOException;
Review comment:
use a default method here to call `unassign(b[])` so that our various
implementations can skip including the same.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java
##########
@@ -508,35 +508,57 @@ default void postAssign(final
ObserverContext<MasterCoprocessorEnvironment> ctx,
* Called prior to unassigning a given region.
* @param ctx the environment to interact with the framework and master
* @param regionInfo
- * @param force whether to force unassignment or not
*/
default void preUnassign(final ObserverContext<MasterCoprocessorEnvironment>
ctx,
- final RegionInfo regionInfo, final boolean force) throws IOException {}
+ final RegionInfo regionInfo) throws IOException {}
/**
* Called after the region unassignment has been requested.
* @param ctx the environment to interact with the framework and master
* @param regionInfo
+ */
+ default void postUnassign(final
ObserverContext<MasterCoprocessorEnvironment> ctx,
+ final RegionInfo regionInfo) throws IOException {}
+
+ /**
+ * Called prior to unassigning a given region.
+ * @param ctx the environment to interact with the framework and master
+ * @param regionInfo region info
+ * @param force whether to force unassignment or not
+ * @deprecated since 2.4.0 and will be removed in 4.0.0.
Review comment:
This class is IA.LimitedPrivate, so we could choose to break compat for
2.4.0 and just remove this method for coprocessors.
If we are keeping it, when should a coprocessor expect this specific version
of `preUnassign` to get called? would it be called instead of the version
without the force parameter or in addition to? These details should be incldued
in the javadoc.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##########
@@ -591,7 +596,11 @@
* @param forcible If true, force unassign (Will remove region from
regions-in-transition too if
* present. If results in double assignment use hbck -fix to
resolve. To be used by
* experts).
+ * @deprecated since 2.4.0 and will be removed in 4.0.0. Use {@link
#unassign(byte[])}
+ * instead.
+ * @see <a
href="https://issues.apache.org/jira/browse/HBASE-24875";>HBASE-24875</a>
*/
+ @Deprecated
CompletableFuture<Void> unassign(byte[] regionName, boolean forcible);
Review comment:
use a default method here to call unassign(b[]) so that our various
implementations can skip including the same.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/UnassignProcedure.java
##########
@@ -80,9 +78,6 @@ protected void serializeStateData(ProcedureStateSerializer
serializer) throws IO
if (this.destinationServer != null) {
state.setDestinationServer(ProtobufUtil.toServerName(destinationServer));
}
- if (force) {
- state.setForce(true);
Review comment:
we should add a comment to the protobuf definition for
`UnassignRegionStateData` in `MasterProcedure` that the field for force is
ignored.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
##########
@@ -1697,7 +1697,6 @@ public UnassignRegionResponse
unassignRegion(RpcController controller,
try {
final byte [] regionName = req.getRegion().getValue().toByteArray();
RegionSpecifierType type = req.getRegion().getType();
- final boolean force = req.getForce();
Review comment:
we should add a comment in the protobuf defition for
`UnassignRegionRequest` in `Master` that the field for force is ignored.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java
##########
@@ -508,35 +508,57 @@ default void postAssign(final
ObserverContext<MasterCoprocessorEnvironment> ctx,
* Called prior to unassigning a given region.
* @param ctx the environment to interact with the framework and master
* @param regionInfo
- * @param force whether to force unassignment or not
*/
default void preUnassign(final ObserverContext<MasterCoprocessorEnvironment>
ctx,
- final RegionInfo regionInfo, final boolean force) throws IOException {}
+ final RegionInfo regionInfo) throws IOException {}
/**
* Called after the region unassignment has been requested.
* @param ctx the environment to interact with the framework and master
* @param regionInfo
+ */
+ default void postUnassign(final
ObserverContext<MasterCoprocessorEnvironment> ctx,
+ final RegionInfo regionInfo) throws IOException {}
+
+ /**
+ * Called prior to unassigning a given region.
+ * @param ctx the environment to interact with the framework and master
+ * @param regionInfo region info
+ * @param force whether to force unassignment or not
+ * @deprecated since 2.4.0 and will be removed in 4.0.0.
+ * Use {@link #preUnassign(ObserverContext, RegionInfo)} instead.
+ */
+ @Deprecated
+ default void preUnassign(final ObserverContext<MasterCoprocessorEnvironment>
ctx,
+ final RegionInfo regionInfo, final boolean force) throws IOException {}
+
+ /**
+ * Called after the region unassignment has been requested.
+ * @param ctx the environment to interact with the framework and master
+ * @param regionInfo region info
* @param force whether to force unassignment or not
+ * @deprecated since 2.4.0 and will be removed in 4.0.0.
Review comment:
This class is IA.LimitedPrivate, so we could choose to break compat for
2.4.0 and just remove this method for coprocessors.
If we are keeping it, when should a coprocessor expect this specific version
of `postUnassign` to get called? would it be called instead of the version
without the force parameter or in addition to? These details should be incldued
in the javadoc.
##########
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##########
@@ -537,8 +537,8 @@ def assign(region_name)
#----------------------------------------------------------------------------------------------
# Unassign a region
- def unassign(region_name, force)
- @admin.unassign(region_name.to_java_bytes,
java.lang.Boolean.valueOf(force))
+ def unassign(region_name)
Review comment:
this breaks compatibility for the shell and will need to be release
noted. Alternatively you could do the analagous thing as in the Admin apis and
deprecate the second parameter by making it optional and stating it is ignored.
##########
File path: hbase-shell/src/main/ruby/shell/commands/unassign.rb
##########
@@ -23,23 +23,18 @@ class Unassign < Command
def help
<<-EOF
Unassign a region. It could be executed only when region in expected
state(OPEN).
-Pass 'true' to force the unassignment ('force' will clear all in-memory state
in
-master before the reassign. If results in double assignment use hbck -fix to
resolve.
-To be used by experts).
In addition, you can use "unassigns" command available on HBCK2 tool to skip
the state check.
(For more info on HBCK2:
https://github.com/apache/hbase-operator-tools/blob/master/hbase-hbck2/README.md)
Use with caution. For experts only.
Examples:
hbase> unassign 'REGIONNAME'
- hbase> unassign 'REGIONNAME', true
hbase> unassign 'ENCODED_REGIONNAME'
- hbase> unassign 'ENCODED_REGIONNAME', true
EOF
end
- def command(region_name, force = 'false')
- admin.unassign(region_name, force)
+ def command(region_name)
Review comment:
this breaks compatibility for the shell and will need to be release
noted. Alternatively you could keep the optional argument and note that it is
deprecated and ignored.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/coprocessor/MasterObserver.java
##########
@@ -508,35 +508,57 @@ default void postAssign(final
ObserverContext<MasterCoprocessorEnvironment> ctx,
* Called prior to unassigning a given region.
* @param ctx the environment to interact with the framework and master
* @param regionInfo
- * @param force whether to force unassignment or not
*/
default void preUnassign(final ObserverContext<MasterCoprocessorEnvironment>
ctx,
- final RegionInfo regionInfo, final boolean force) throws IOException {}
+ final RegionInfo regionInfo) throws IOException {}
Review comment:
If we keep both `preUnassign` methods then this default method should
call the one that includes a force flag, with the flag set to `false`. That
would allow existing coprocessors to be used as-is without recompiling, would
avoid needing to make a second call in the `MasterCoprocessorHost`, and would
be easy to document in the javadoc for the deprecation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
