ndimiduk commented on a change in pull request #4180:
URL: https://github.com/apache/hbase/pull/4180#discussion_r834334898
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -798,9 +798,19 @@
/**
* Parameter name for client pause value for special case such as call queue
too big, etc.
+ * @deprecated Since 2.6.0, will be removed in 4.0.0. Please use
Review comment:
Ah good, you've done exactly this. Except, if you're quick, this will go
out in 2.5.0.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncConnectionConfiguration.java
##########
@@ -31,6 +31,7 @@
import static
org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_OPERATION_TIMEOUT;
import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_PAUSE;
import static org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_PAUSE_FOR_CQTBE;
+import static
org.apache.hadoop.hbase.HConstants.HBASE_CLIENT_PAUSE_FOR_SERVER_OVERLOADED;
Review comment:
This was the source of your discussion around constants? And you still
decided to put it in `HConstants`? :(
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -798,9 +798,19 @@
/**
* Parameter name for client pause value for special case such as call queue
too big, etc.
+ * @deprecated Since 2.6.0, will be removed in 4.0.0. Please use
+ * hbase.client.pause.server.overloaded instead.
*/
+ @Deprecated
Review comment:
You also should add a mapping between these values via
`Configuration#addDeprecation()`. I'm actually shocked that I cannot find any
uses of this method in the HBase codebase. I was hoping there would be an
obvious place to add it. Maybe someone else can review and suggest where.
Basically, I think we need a static code block someplace where the deprecation
can be registered once. Perhaps in the static initializer of the class where
it's used, `AsyncConnectionConfiguration` ?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncClientPauseForServerOverloaded.java
##########
@@ -56,31 +58,44 @@
import
org.apache.hbase.thirdparty.com.google.protobuf.Descriptors.MethodDescriptor;
@Category({ MediumTests.class, ClientTests.class })
-public class TestAsyncClientPauseForCallQueueTooBig {
+public class TestAsyncClientPauseForServerOverloaded {
Review comment:
Nice test improvements, checking both failure classes. Please also add
coverage to check that when the client provides the old value, it is honored
(and perhaps also a warning is produced... the `Configuration.addDeprecation`
may log the warning for you, I don't remember.)
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/ServerOverloadedException.java
##########
@@ -0,0 +1,33 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase;
+
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Base class for exceptions thrown when the hbase server is overloaded.
+ */
[email protected]
+public class ServerOverloadedException extends HBaseIOException {
Review comment:
A note for other reviewers: I think that this inheritance hierarchy
should be fine from a backward compatibility perspective, because
`HBaseIOException` extends from `IOException`.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdminBuilder.java
##########
@@ -50,21 +50,21 @@
* Set the base pause time for retrying. We use an exponential policy to
generate sleep time when
* retrying.
* @return this for invocation chaining
- * @see #setRetryPauseForCQTBE(long, TimeUnit)
+ * @see #setRetryPauseForServerOverloaded(long, TimeUnit)
*/
AsyncAdminBuilder setRetryPause(long timeout, TimeUnit unit);
/**
- * Set the base pause time for retrying when we hit {@code
CallQueueTooBigException}. We use an
+ * Set the base pause time for retrying when we hit {@code
ServerOverloadedException}. We use an
* exponential policy to generate sleep time when retrying.
* <p/>
* This value should be greater than the normal pause value which could be
set with the above
- * {@link #setRetryPause(long, TimeUnit)} method, as usually {@code
CallQueueTooBigException}
+ * {@link #setRetryPause(long, TimeUnit)} method, as usually {@code
ServerOverloadedException}
* means the server is overloaded. We just use the normal pause value for
- * {@code CallQueueTooBigException} if here you specify a smaller value.
+ * {@code ServerOverloadedException} if here you specify a smaller value.
* @see #setRetryPause(long, TimeUnit)
*/
- AsyncAdminBuilder setRetryPauseForCQTBE(long pause, TimeUnit unit);
Review comment:
You must retain the old method signature for backward compatibility
purposes. Annotate it as deprecated as of some version, and mention a target
removal version. I think for this, it would be deprecated as of 2.x.0, target
removal for 4.0.0.
It seems fine for the old to delegate to the new.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/exceptions/ClientExceptionsUtil.java
##########
@@ -111,28 +111,6 @@ public static Throwable findException(Object exception) {
return null;
}
- /**
- * Checks if the exception is CallQueueTooBig exception (maybe wrapped
- * into some RemoteException).
- * @param t exception to check
- * @return true if it's a CQTBE, false otherwise
- */
- public static boolean isCallQueueTooBigException(Throwable t) {
Review comment:
👏
##########
File path: hbase-common/src/main/resources/hbase-default.xml
##########
@@ -493,12 +493,13 @@ possible configurations would overwhelm and obscure the
important.
this initial pause amount and how this pause works w/
retries.</description>
</property>
<property>
- <name>hbase.client.pause.cqtbe</name>
+ <name>hbase.client.pause.server.overloaded</name>
<value></value>
- <description>Whether or not to use a special client pause for
- CallQueueTooBigException (cqtbe). Set this property to a higher value
- than hbase.client.pause if you observe frequent CQTBE from the same
- RegionServer and the call queue there keeps full</description>
+ <description>Pause time when encountering an exception indicating a
+ server is overloaded, CallQueueTooBigException or CallDroppedException.
+ Set this property to a higher value than hbase.client.pause if you
+ observe frequent CallQueueTooBigException or CallDroppedException from the
same
+ RegionServer and the call queue there keeps filling up</description>
Review comment:
Add a comment here also saying that this configuration was previously
called `hbase.client.pause.cqtbe`. I recommend this because hbase-default.xml
is rendered into the online book.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
