You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/08/14 04:25:57 UTC
[GitHub] [hbase] busbey commented on a change in pull request #2254: HBASE-24875 Remove the force param for unassign since it dose not take effect any more
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:
users@infra.apache.org