Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HBASE-26727 Fix CallDroppedException reporting #4088

Merged
merged 1 commit into from
Feb 19, 2022
Merged

HBASE-26727 Fix CallDroppedException reporting #4088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...-hadoop-compat/src/main/java/org/apache/hadoop/hbase/metrics/ExceptionTrackingSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public interface ExceptionTrackingSource extends BaseSource {
String EXCEPTIONS_QUOTA_EXCEEDED = "exceptions.quotaExceeded";
String EXCEPTIONS_RPC_THROTTLING = "exceptions.rpcThrottling";
String EXCEPTIONS_CALL_DROPPED = "exceptions.callDropped";
String EXCEPTIONS_CALL_TIMED_OUT = "exceptions.callTimedOut";

void exception();

Expand All @@ -62,4 +63,5 @@ public interface ExceptionTrackingSource extends BaseSource {
void quotaExceededException();
void rpcThrottlingException();
void callDroppedException();
void callTimedOut();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this in this PR because it's a type of call drop that can happen a lot under excess load. I did not roll it into callDroppedException, because it does not manifest as a CallDroppedException on the client side.

}
8 changes: 8 additions & 0 deletions ...oop-compat/src/main/java/org/apache/hadoop/hbase/metrics/ExceptionTrackingSourceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public class ExceptionTrackingSourceImpl extends BaseSourceImpl
protected MutableFastCounter exceptionsQuotaExceeded;
protected MutableFastCounter exceptionsRpcThrottling;
protected MutableFastCounter exceptionsCallDropped;
protected MutableFastCounter exceptionsCallTimedOut;

public ExceptionTrackingSourceImpl(String metricsName, String metricsDescription,
String metricsContext, String metricsJmxContext) {
Expand Down Expand Up @@ -75,6 +76,8 @@ public void init() {
.newCounter(EXCEPTIONS_RPC_THROTTLING, EXCEPTIONS_TYPE_DESC, 0L);
this.exceptionsCallDropped = this.getMetricsRegistry()
.newCounter(EXCEPTIONS_CALL_DROPPED, EXCEPTIONS_TYPE_DESC, 0L);
this.exceptionsCallTimedOut = this.getMetricsRegistry()
.newCounter(EXCEPTIONS_CALL_TIMED_OUT, EXCEPTIONS_TYPE_DESC, 0L);
}

@Override
Expand Down Expand Up @@ -141,4 +144,9 @@ public void rpcThrottlingException() {
public void callDroppedException() {
exceptionsCallDropped.incr();
}

@Override
public void callTimedOut() {
exceptionsCallTimedOut.incr();
}
}
2 changes: 2 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/CallRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public void run() {
call.setStartTime(EnvironmentEdgeManager.currentTime());
if (call.getStartTime() > call.getDeadline()) {
RpcServer.LOG.warn("Dropping timed out call: " + call);
this.rpcServer.getMetrics().callTimedOut();
return;
}
this.status.setStatus("Setting up call");
Expand Down Expand Up @@ -222,6 +223,7 @@ public void drop() {
call.setResponse(null, null, CALL_DROPPED_EXCEPTION, "Call dropped, server "
+ (address != null ? address : "(channel closed)") + " is overloaded, please retry.");
call.sendResponseIfReady();
this.rpcServer.getMetrics().exception(CALL_DROPPED_EXCEPTION);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we call this method in the past? Will this introduce double accounting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, we do not report them. We added this exception in https://issues.apache.org/jira/browse/HBASE-26623, but the problem is when we drop a call here it doesn't end up hitting any of the other invocations of this method. So the value is always 0 despite throwing these exceptions to clients.

Copy link
Contributor Author

@bbeaudreault bbeaudreault Feb 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically this is called in a few places, for example in RpcServer or in a few try/catches in RSRpcServices. But a couple exceptions need to be handled a level above this. For example CallQueueTooBigException is handled in the connection and RequestTooBig in the netty frame decoder.

So this exception is another example of a special case like CallQueueTooBig/RequestTooBig, that doesn't get handled by the default try/catches we have in RpcServer/RSRpcServices.

} catch (ClosedChannelException cce) {
InetSocketAddress address = rpcServer.getListenerAddress();
RpcServer.LOG.warn(Thread.currentThread().getName() + ": caught a ClosedChannelException, " +
Expand Down
4 changes: 4 additions & 0 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/ipc/MetricsHBaseServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@ public void exception(Throwable throwable) {
}
}

void callTimedOut() {
source.callTimedOut();
}

public MetricsHBaseServerSource getMetricsSource() {
return source;
}
Expand Down
21 changes: 20 additions & 1 deletion hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestCallRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
*/
package org.apache.hadoop.hbase.ipc;

import java.net.InetSocketAddress;
import org.apache.hadoop.hbase.CallDroppedException;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.monitoring.MonitoredRPCHandlerImpl;
import org.apache.hadoop.hbase.testclassification.RPCTests;
Expand Down Expand Up @@ -60,7 +62,7 @@ public void testCallCleanup() {
}

@Test
public void testCallRunnerDrop() {
public void testCallRunnerDropDisconnected() {
RpcServerInterface mockRpcServer = Mockito.mock(RpcServerInterface.class);
Mockito.when(mockRpcServer.isStarted()).thenReturn(true);
ServerCall mockCall = Mockito.mock(ServerCall.class);
Expand All @@ -71,4 +73,21 @@ public void testCallRunnerDrop() {
cr.drop();
Mockito.verify(mockCall, Mockito.times(1)).cleanup();
}

@Test
public void testCallRunnerDropConnected() {
RpcServerInterface mockRpcServer = Mockito.mock(RpcServerInterface.class);
MetricsHBaseServer mockMetrics = Mockito.mock(MetricsHBaseServer.class);
Mockito.when(mockRpcServer.getMetrics()).thenReturn(mockMetrics);
Mockito.when(mockRpcServer.isStarted()).thenReturn(true);
Mockito.when(mockRpcServer.getListenerAddress()).thenReturn(InetSocketAddress.createUnresolved("foo", 60020));
ServerCall mockCall = Mockito.mock(ServerCall.class);
Mockito.when(mockCall.disconnectSince()).thenReturn(-1L);

CallRunner cr = new CallRunner(mockRpcServer, mockCall);
cr.setStatus(new MonitoredRPCHandlerImpl());
cr.drop();
Mockito.verify(mockCall, Mockito.times(1)).cleanup();
Mockito.verify(mockMetrics).exception(Mockito.any(CallDroppedException.class));
}
}