Apache9 commented on code in PR #5275: URL: https://github.com/apache/hbase/pull/5275#discussion_r1225754851
########## hbase-client/src/main/java/org/apache/hadoop/hbase/client/backoff/HBaseServerExceptionPauseManager.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.client.backoff; + +import java.util.OptionalLong; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.hbase.HBaseServerException; +import org.apache.hadoop.hbase.quotas.RpcThrottlingException; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected] +public class HBaseServerExceptionPauseManager { + private static final Logger LOG = LoggerFactory.getLogger(HBaseServerExceptionPauseManager.class); + + private final long pauseNs; + private final long pauseNsForServerOverloaded; + + public HBaseServerExceptionPauseManager(long pauseNs, long pauseNsForServerOverloaded) { + this.pauseNs = pauseNs; + this.pauseNsForServerOverloaded = pauseNsForServerOverloaded; + } + + public OptionalLong getPauseNsFromException(Throwable error, long remainingTimeNs) { + long expectedSleepNs; + if (error instanceof RpcThrottlingException) { + RpcThrottlingException rpcThrottlingException = (RpcThrottlingException) error; + expectedSleepNs = TimeUnit.MILLISECONDS.toNanos(rpcThrottlingException.getWaitInterval()); + if (expectedSleepNs > remainingTimeNs) { + return OptionalLong.empty(); + } + if (LOG.isDebugEnabled()) { + LOG.debug("Sleeping for {}ms after catching RpcThrottlingException", expectedSleepNs, + rpcThrottlingException); + } + } else { + expectedSleepNs = Review Comment: Here it means pauseNs, which is a base value for calculating the sleep time, but for the above value which extract from the RpcThrottlingException, it does means the expected sleep time, not a base value. SO I think here we need to put more values in this method, and return expected sleep time? Otherwise it may not act as we expect, for example, the RpcThrottlingException tells us to retry after 1 second, but then we will use 1 second as pause, and multiply a number which is determined by the retry time, as the sleep time, it may lead to a very long sleep time, such as several tens of seconds... ########## hbase-client/src/main/java/org/apache/hadoop/hbase/client/backoff/HBaseServerExceptionPauseManager.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.client.backoff; + +import java.util.OptionalLong; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.hbase.HBaseServerException; +import org.apache.hadoop.hbase.quotas.RpcThrottlingException; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected] +public class HBaseServerExceptionPauseManager { + private static final Logger LOG = LoggerFactory.getLogger(HBaseServerExceptionPauseManager.class); + + private final long pauseNs; + private final long pauseNsForServerOverloaded; + + public HBaseServerExceptionPauseManager(long pauseNs, long pauseNsForServerOverloaded) { + this.pauseNs = pauseNs; + this.pauseNsForServerOverloaded = pauseNsForServerOverloaded; + } + + public OptionalLong getPauseNsFromException(Throwable error, long remainingTimeNs) { Review Comment: Please add javadoc here to describe the meaning of the return value. For me, I think maybe return long directly is a better choice, as we could return -1 if we should fail. Returning OptionalLong seems indicating that, if we return OptionalLong.empty, the upper layer should decide the pauseNs by their own? ########## hbase-client/src/main/java/org/apache/hadoop/hbase/client/backoff/HBaseServerExceptionPauseManager.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.client.backoff; + +import java.util.OptionalLong; +import java.util.concurrent.TimeUnit; +import org.apache.hadoop.hbase.HBaseServerException; +import org.apache.hadoop.hbase.quotas.RpcThrottlingException; +import org.apache.yetus.audience.InterfaceAudience; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + [email protected] +public class HBaseServerExceptionPauseManager { + private static final Logger LOG = LoggerFactory.getLogger(HBaseServerExceptionPauseManager.class); + + private final long pauseNs; + private final long pauseNsForServerOverloaded; + + public HBaseServerExceptionPauseManager(long pauseNs, long pauseNsForServerOverloaded) { + this.pauseNs = pauseNs; + this.pauseNsForServerOverloaded = pauseNsForServerOverloaded; + } + + public OptionalLong getPauseNsFromException(Throwable error, long remainingTimeNs) { + long expectedSleepNs; + if (error instanceof RpcThrottlingException) { + RpcThrottlingException rpcThrottlingException = (RpcThrottlingException) error; + expectedSleepNs = TimeUnit.MILLISECONDS.toNanos(rpcThrottlingException.getWaitInterval()); + if (expectedSleepNs > remainingTimeNs) { Review Comment: Better add a log here to mention that we will give up retrying since the remaining time is not enough for the next retry because of the server is throttling us. -- 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]
