Skip to content

Commit

Permalink
HBASE-24294 [Flakey Tests] TestThriftHttpServer BindException (#1619)
Browse files Browse the repository at this point in the history 
Refactor Thrift tests so resilient when launch of ThriftServer runs
into BindException.

hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestBindExceptionHandling.java
hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftHttpServer.java
hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServerCmdLine.java
hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThrift2HttpServer.java
hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift2/TestThrift2ServerCmdLine.java
 Refactor to use new ThriftServerRunner and ThriftServerSupplier

hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
 Log list of existing tables on assert.

hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpFallbackServer.java
hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftSpnegoHttpServer.java
 Add Ignore for testRunThriftServerWithHeaderBufferLength. Its tested in
 superclass. No need to test again here. Besides, its destructive test
 leaving behind dregs that mess up the retry.

hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/ThriftServerRunner.java
 Utility. Thread to host ThriftServer and exception if one after run
 completes.
  • Loading branch information
@saintstack
saintstack committed May 1, 2020
1 parent 4262c72 commit a3c8ec8
Show file tree
Hide file tree
Showing 9 changed files with 248 additions and 191 deletions.
32 changes: 12 additions & 20 deletions hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestBindExceptionHandling.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,49 +19,41 @@

import static org.junit.Assert.assertNotNull;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.junit.ClassRule;
import org.junit.Test;
import org.junit.experimental.categories.Category;
import java.io.IOException;

@Category({ ClientTests.class, MediumTests.class})
public class TestBindExceptionHandling {
@ClassRule
public static final HBaseClassTestRule CLASS_RULE =
HBaseClassTestRule.forClass(TestBindExceptionHandling.class);

private static final HBaseTestingUtility HTU = new HBaseTestingUtility();

/**
* See if random port choosing works around protocol port clashes
*/
@Test
public void testProtocolPortClash() {
ThriftServer thriftServer = null;
try {
thriftServer = new TestThriftServerCmdLine(null, false, false, false).
createBoundServer(true, false);
assertNotNull(thriftServer.tserver);
} finally {
if (thriftServer != null) {
thriftServer.stop();
}
public void testProtocolPortClash() throws Exception {
try (ThriftServerRunner tsr = TestThriftServerCmdLine.
createBoundServer(() -> new ThriftServer(HTU.getConfiguration()), true, false)) {
assertNotNull(tsr.getThriftServer());
}
}

/**
* See if random port choosing works around protocol port clashes
*/
@Test
public void testInfoPortClash() {
ThriftServer thriftServer = null;
try {
thriftServer = new TestThriftServerCmdLine(null, false, false, false).
createBoundServer(false, true);
assertNotNull(thriftServer.tserver);
} finally {
if (thriftServer != null) {
thriftServer.stop();
}
public void testInfoPortClash() throws Exception {
try (ThriftServerRunner tsr = TestThriftServerCmdLine.
createBoundServer(() -> new ThriftServer(HTU.getConfiguration()), false, true)) {
assertNotNull(tsr.getThriftServer());
}
}
}
119 changes: 33 additions & 86 deletions hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftHttpServer.java
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
* 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
Expand All @@ -17,16 +17,14 @@
*/
package org.apache.hadoop.hbase.thrift;

import static org.apache.hadoop.hbase.thrift.Constants.INFOPORT_OPTION;
import static org.apache.hadoop.hbase.thrift.Constants.PORT_OPTION;
import static org.apache.hadoop.hbase.thrift.TestThriftServerCmdLine.createBoundServer;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.fail;

import java.net.BindException;
import java.io.IOException;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;
import java.util.function.Supplier;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
Expand Down Expand Up @@ -70,17 +68,9 @@ public class TestThriftHttpServer {

protected static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility();

private Thread httpServerThread;
private volatile Exception httpServerException;

private Exception clientSideException;

private ThriftServer thriftServer;
int port;

@BeforeClass
public static void setUpBeforeClass() throws Exception {
TEST_UTIL.getConfiguration().setBoolean("hbase.regionserver.thrift.http", true);
TEST_UTIL.getConfiguration().setBoolean(Constants.USE_HTTP_CONF_KEY, true);
TEST_UTIL.getConfiguration().setBoolean(TableDescriptorChecker.TABLE_SANITY_CHECKS, false);
TEST_UTIL.startMiniCluster();
//ensure that server time increments every time we do an operation, otherwise
Expand All @@ -95,45 +85,32 @@ public static void tearDownAfterClass() throws Exception {
}

@Test
public void testExceptionThrownWhenMisConfigured() throws Exception {
public void testExceptionThrownWhenMisConfigured() throws IOException {
Configuration conf = new Configuration(TEST_UTIL.getConfiguration());
conf.set("hbase.thrift.security.qop", "privacy");
conf.setBoolean("hbase.thrift.ssl.enabled", false);

ThriftServer server = null;
ExpectedException thrown = ExpectedException.none();
ThriftServerRunner tsr = null;
try {
thrown.expect(IllegalArgumentException.class);
thrown.expectMessage("Thrift HTTP Server's QoP is privacy, " +
"but hbase.thrift.ssl.enabled is false");
server = new ThriftServer(conf);
server.run();
tsr = TestThriftServerCmdLine.createBoundServer(() -> new ThriftServer(conf));
fail("Thrift HTTP Server starts up even with wrong security configurations.");
} catch (Exception e) {
}
}

private void startHttpServerThread(final String[] args) {
LOG.info("Starting HBase Thrift server with HTTP server: " + Joiner.on(" ").join(args));

httpServerException = null;
httpServerThread = new Thread(() -> {
try {
thriftServer.run(args);
} catch (Exception e) {
httpServerException = e;
LOG.info("Expected!", e);
} finally {
if (tsr != null) {
tsr.close();
}
});
httpServerThread.setName(ThriftServer.class.getSimpleName() + "-httpServer");
httpServerThread.start();
}
}

@Rule
public ExpectedException exception = ExpectedException.none();

@Test
public void testRunThriftServerWithHeaderBufferLength() throws Exception {

// Test thrift server with HTTP header length less than 64k
try {
runThriftServer(1024 * 63);
Expand All @@ -147,8 +124,8 @@ public void testRunThriftServerWithHeaderBufferLength() throws Exception {
runThriftServer(1024 * 64);
}

protected ThriftServer createThriftServer() {
return new ThriftServer(TEST_UTIL.getConfiguration());
protected Supplier<ThriftServer> getThriftServerSupplier() {
return () -> new ThriftServer(TEST_UTIL.getConfiguration());
}

@Test
Expand All @@ -157,48 +134,33 @@ public void testRunThriftServer() throws Exception {
}

void runThriftServer(int customHeaderSize) throws Exception {
for (int i = 0; i < 100; i++) {
List<String> args = new ArrayList<>(3);
port = HBaseTestingUtility.randomFreePort();
args.add("-" + PORT_OPTION);
args.add(String.valueOf(port));
args.add("-" + INFOPORT_OPTION);
int infoPort = HBaseTestingUtility.randomFreePort();
args.add(String.valueOf(infoPort));
args.add("start");

thriftServer = createThriftServer();
startHttpServerThread(args.toArray(new String[args.size()]));

// wait up to 10s for the server to start
HBaseTestingUtility.waitForHostPort(HConstants.LOCALHOST, port);

String url = "http://" + HConstants.LOCALHOST + ":" + port;
// Add retries in case we see stuff like connection reset
Exception clientSideException = null;
for (int i = 0; i < 10; i++) {
clientSideException = null;
ThriftServerRunner tsr = createBoundServer(getThriftServerSupplier());
String url = "http://" + HConstants.LOCALHOST + ":" + tsr.getThriftServer().listenPort;
try {
checkHttpMethods(url);
talkToThriftServer(url, customHeaderSize);
break;
} catch (Exception ex) {
clientSideException = ex;
LOG.info("Client-side Exception", ex);
} finally {
stopHttpServerThread();
}

if (clientSideException != null) {
LOG.error("Thrift client threw an exception " + clientSideException);
if (clientSideException instanceof TTransportException) {
if (clientSideException.getCause() != null &&
clientSideException.getCause() instanceof BindException) {
continue;
}
throw clientSideException;
} else {
throw new Exception(clientSideException);
tsr.close();
tsr.join();
if (tsr.getRunException() != null) {
LOG.error("Invocation of HBase Thrift server threw exception", tsr.getRunException());
throw tsr.getRunException();
}
} else {
// Done.
break;
}
}

if (clientSideException != null) {
LOG.error("Thrift Client", clientSideException);
throw clientSideException;
}
}

private void checkHttpMethods(String url) throws Exception {
Expand Down Expand Up @@ -238,19 +200,4 @@ protected void talkToThriftServer(String url, int customHeaderSize) throws Excep
httpClient.close();
}
}

private void stopHttpServerThread() throws Exception {
LOG.debug("Stopping Thrift HTTP server {}", thriftServer);
if (thriftServer != null) {
thriftServer.stop();
}
if (httpServerThread != null) {
httpServerThread.join();
}
if (httpServerException != null) {
LOG.error("Command-line invocation of HBase Thrift server threw an " +
"exception", httpServerException);
throw new Exception(httpServerException);
}
}
}
7 changes: 5 additions & 2 deletions hbase-thrift/src/test/java/org/apache/hadoop/hbase/thrift/TestThriftServer.java
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/**
/*
* 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
Expand Down Expand Up @@ -32,6 +32,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.CompatibilityFactory;
import org.apache.hadoop.hbase.HBaseClassTestRule;
Expand Down Expand Up @@ -254,7 +255,9 @@ private static ThriftMetrics getMetrics(Configuration conf) throws Exception {

public static void createTestTables(Hbase.Iface handler) throws Exception {
// Create/enable/disable/delete tables, ensure methods act correctly
assertEquals(0, handler.getTableNames().size());
List<java.nio.ByteBuffer> bbs = handler.getTableNames();
assertEquals(bbs.stream().map(b -> Bytes.toString(b.array())).
collect(Collectors.joining(",")), 0, bbs.size());
handler.createTable(tableAname, getColumnDescriptors());
assertEquals(1, handler.getTableNames().size());
assertEquals(2, handler.getColumnDescriptors(tableAname).size());
Expand Down
Loading

0 comments on commit a3c8ec8

Please sign in to comment.