Skip to content

Commit 0f84363

Browse files
bharathvbharathv
committed
HBASE-23731: De-flake TestFromClientSide (#1091)
There were a couple of issues. - There was a leak of a file descriptor for hbck lock file. This was contributing to all the "ConnectionRefused" stack traces since it was trying to renew lease for an already expired mini dfs cluster. This issue was there for a while, just that we noticed it now. - After upgrade to JUnit 4.13, it looks like the behavior for test timeouts has changed. Earlier the timeout seems to have applied for each parameterized run, but now it looks like it is applied across all the runs. This patch fixes both the issues. Signed-off-by: Stack <stack@apache.org> Signed-off-by: Jan Hentschel <jan.hentschel@ultratendency.com>
1 parent 8984f86 commit 0f84363

3 files changed

Lines changed: 234 additions & 11 deletions

File tree

‎hbase-common/src/test/java/org/apache/hadoop/hbase/HBaseClassTestRule.java‎

Lines changed: 78 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/**
1+
/*
22
* Licensed to the Apache Software Foundation (ASF) under one
33
* or more contributor license agreements. See the NOTICE file
44
* distributed with this work for additional information
@@ -17,10 +17,16 @@
1717
*/
1818
package org.apache.hadoop.hbase;
1919

20+
import edu.umd.cs.findbugs.annotations.NonNull;
21+
import java.lang.reflect.InvocationTargetException;
22+
import java.lang.reflect.Method;
23+
import java.lang.reflect.Modifier;
24+
import java.util.Arrays;
25+
import java.util.Collection;
2026
import java.util.Collections;
27+
import java.util.List;
2128
import java.util.Set;
2229
import java.util.concurrent.TimeUnit;
23-
2430
import org.apache.hadoop.hbase.testclassification.IntegrationTests;
2531
import org.apache.hadoop.hbase.testclassification.LargeTests;
2632
import org.apache.hadoop.hbase.testclassification.MediumTests;
@@ -30,8 +36,14 @@
3036
import org.junit.rules.TestRule;
3137
import org.junit.rules.Timeout;
3238
import org.junit.runner.Description;
39+
import org.junit.runner.RunWith;
40+
import org.junit.runners.Parameterized;
41+
import org.junit.runners.Parameterized.Parameters;
3342
import org.junit.runners.model.Statement;
34-
43+
import org.slf4j.Logger;
44+
import org.slf4j.LoggerFactory;
45+
import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
46+
import org.apache.hbase.thirdparty.com.google.common.collect.Iterables;
3547
import org.apache.hbase.thirdparty.com.google.common.collect.Sets;
3648

3749
/**
@@ -43,9 +55,13 @@
4355
*/
4456
@InterfaceAudience.Private
4557
public final class HBaseClassTestRule implements TestRule {
58+
private static final Logger LOG = LoggerFactory.getLogger(HBaseClassTestRule.class);
4659
public static final Set<Class<?>> UNIT_TEST_CLASSES = Collections.unmodifiableSet(
4760
Sets.<Class<?>> newHashSet(SmallTests.class, MediumTests.class, LargeTests.class));
4861

62+
// Each unit test has this timeout.
63+
private static long PER_UNIT_TEST_TIMEOUT_MINS = 13;
64+
4965
private final Class<?> clazz;
5066

5167
private final Timeout timeout;
@@ -65,13 +81,16 @@ public Class<?> getClazz() {
6581

6682
private static long getTimeoutInSeconds(Class<?> clazz) {
6783
Category[] categories = clazz.getAnnotationsByType(Category.class);
68-
84+
// Starting JUnit 4.13, it appears that the timeout is applied across all the parameterized
85+
// runs. So the timeout is multiplied by number of parameterized runs.
86+
int numParams = getNumParameters(clazz);
6987
// @Category is not repeatable -- it is only possible to get an array of length zero or one.
7088
if (categories.length == 1) {
7189
for (Class<?> c : categories[0].value()) {
7290
if (UNIT_TEST_CLASSES.contains(c)) {
73-
// All tests have a 13 minutes timeout.
74-
return TimeUnit.MINUTES.toSeconds(13);
91+
long timeout = numParams * PER_UNIT_TEST_TIMEOUT_MINS;
92+
LOG.info("Test {} timeout: {} mins", clazz, timeout);
93+
return TimeUnit.MINUTES.toSeconds(timeout);
7594
}
7695
if (c == IntegrationTests.class) {
7796
return TimeUnit.MINUTES.toSeconds(Long.MAX_VALUE);
@@ -82,6 +101,59 @@ private static long getTimeoutInSeconds(Class<?> clazz) {
82101
clazz.getName() + " does not have SmallTests/MediumTests/LargeTests in @Category");
83102
}
84103

104+
/**
105+
* @param clazz Test class that is running.
106+
* @return the number of parameters for this given test class. If the test is not parameterized or
107+
* if there is any issue determining the number of parameters, returns 1.
108+
*/
109+
@VisibleForTesting
110+
static int getNumParameters(Class<?> clazz) {
111+
RunWith[] runWiths = clazz.getAnnotationsByType(RunWith.class);
112+
boolean testParameterized = runWiths != null && Arrays.stream(runWiths).anyMatch(
113+
(r) -> r.value().equals(Parameterized.class));
114+
if (!testParameterized) {
115+
return 1;
116+
}
117+
for (Method method : clazz.getMethods()) {
118+
if (!isParametersMethod(method)) {
119+
continue;
120+
}
121+
// Found the parameters method. Figure out the number of parameters.
122+
Object parameters;
123+
try {
124+
parameters = method.invoke(clazz);
125+
} catch (IllegalAccessException | InvocationTargetException e) {
126+
LOG.warn("Error invoking parameters method {} in test class {}",
127+
method.getName(), clazz, e);
128+
continue;
129+
}
130+
if (parameters instanceof List) {
131+
return ((List) parameters).size();
132+
} else if (parameters instanceof Collection) {
133+
return ((Collection) parameters).size();
134+
} else if (parameters instanceof Iterable) {
135+
return Iterables.size((Iterable) parameters);
136+
} else if (parameters instanceof Object[]) {
137+
return ((Object[]) parameters).length;
138+
}
139+
}
140+
LOG.warn("Unable to determine parameters size. Returning the default of 1.");
141+
return 1;
142+
}
143+
144+
/**
145+
* Helper method that checks if the input method is a valid JUnit @Parameters method.
146+
* @param method Input method.
147+
* @return true if the method is a valid JUnit parameters method, false otherwise.
148+
*/
149+
private static boolean isParametersMethod(@NonNull Method method) {
150+
// A valid parameters method is public static and with @Parameters annotation.
151+
boolean methodPublicStatic = Modifier.isPublic(method.getModifiers()) &&
152+
Modifier.isStatic(method.getModifiers());
153+
Parameters[] params = method.getAnnotationsByType(Parameters.class);
154+
return methodPublicStatic && (params != null && params.length > 0);
155+
}
156+
85157
public static HBaseClassTestRule forClass(Class<?> clazz) {
86158
return new HBaseClassTestRule(clazz, Timeout.builder().withLookingForStuckThread(true)
87159
.withTimeout(getTimeoutInSeconds(clazz), TimeUnit.SECONDS).build());

‎hbase-common/src/test/java/org/apache/hadoop/hbase/TestHBaseClassTestRule.java‎

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,145 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
package org.apache.hadoop.hbase;
19+
20+
import static junit.framework.TestCase.assertEquals;
21+
import java.util.Arrays;
22+
import java.util.Collection;
23+
import java.util.List;
24+
import org.apache.hadoop.hbase.testclassification.SmallTests;
25+
import org.junit.ClassRule;
26+
import org.junit.Test;
27+
import org.junit.experimental.categories.Category;
28+
import org.junit.runner.RunWith;
29+
import org.junit.runners.Parameterized;
30+
import org.junit.runners.Parameterized.Parameters;
31+
import org.apache.hbase.thirdparty.com.google.common.collect.Iterables;
32+
33+
/**
34+
* Tests HBaseClassTestRule.
35+
*/
36+
@Category(SmallTests.class)
37+
public class TestHBaseClassTestRule {
38+
39+
@ClassRule
40+
public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule.forClass(
41+
TestHBaseClassTestRule.class);
42+
43+
// Test input classes of various kinds.
44+
private static class NonParameterizedClass {
45+
void dummy() {
46+
}
47+
int dummy(int a) {
48+
return 0;
49+
}
50+
}
51+
52+
@RunWith(Parameterized.class)
53+
private static class ParameterizedClassWithNoParametersMethod {
54+
void dummy() {
55+
}
56+
}
57+
58+
@RunWith(Parameterized.class)
59+
private static class InValidParameterizedClass {
60+
// Not valid because parameters method is private.
61+
@Parameters
62+
private static List<Object> parameters() {
63+
return Arrays.asList(1, 2, 3, 4);
64+
}
65+
int dummy(int a) {
66+
return 0;
67+
}
68+
}
69+
70+
@RunWith(Parameterized.class)
71+
private static class ValidParameterizedClass1 {
72+
@Parameters
73+
public static List<Object> parameters() {
74+
return Arrays.asList(1, 2, 3, 4, 5);
75+
}
76+
int dummy(int a) {
77+
return 0;
78+
}
79+
}
80+
81+
@RunWith(Parameterized.class)
82+
private static class ValidParameterizedClass2 {
83+
@Parameters
84+
public static Object[] parameters() {
85+
return new Integer[] {1, 2, 3, 4, 5, 6};
86+
}
87+
}
88+
89+
@RunWith(Parameterized.class)
90+
private static class ValidParameterizedClass3 {
91+
@Parameters
92+
public static Iterable<Integer> parameters() {
93+
return Arrays.asList(1, 2, 3, 4, 5, 6, 7);
94+
}
95+
}
96+
97+
@RunWith(Parameterized.class)
98+
private static class ValidParameterizedClass4 {
99+
@Parameters
100+
public static Collection<Integer> parameters() {
101+
return Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9);
102+
}
103+
}
104+
105+
106+
@RunWith(Parameterized.class)
107+
private static class ExtendedParameterizedClass1 extends ValidParameterizedClass1 {
108+
// Should be inferred from the parent class.
109+
int dummy2(int a) {
110+
return 0;
111+
}
112+
}
113+
114+
@RunWith(Parameterized.class)
115+
private static class ExtendedParameterizedClass2 extends ValidParameterizedClass1 {
116+
// Should override the parent parameters class.
117+
@Parameters
118+
public static List<Object> parameters() {
119+
return Arrays.asList(1, 2, 3);
120+
}
121+
}
122+
123+
@Test
124+
public void testNumParameters() {
125+
// Invalid cases, expected to return 1.
126+
assertEquals(HBaseClassTestRule.getNumParameters(NonParameterizedClass.class), 1);
127+
assertEquals(HBaseClassTestRule.getNumParameters(
128+
ParameterizedClassWithNoParametersMethod.class), 1);
129+
assertEquals(HBaseClassTestRule.getNumParameters(InValidParameterizedClass.class), 1);
130+
// Valid parameterized classes.
131+
assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass1.class),
132+
ValidParameterizedClass1.parameters().size());
133+
assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass2.class),
134+
ValidParameterizedClass2.parameters().length);
135+
assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass3.class),
136+
Iterables.size(ValidParameterizedClass3.parameters()));
137+
assertEquals(HBaseClassTestRule.getNumParameters(ValidParameterizedClass4.class),
138+
ValidParameterizedClass4.parameters().size());
139+
// Testing inheritance.
140+
assertEquals(HBaseClassTestRule.getNumParameters(ExtendedParameterizedClass1.class),
141+
ValidParameterizedClass1.parameters().size());
142+
assertEquals(HBaseClassTestRule.getNumParameters(ExtendedParameterizedClass2.class),
143+
ExtendedParameterizedClass2.parameters().size());
144+
}
145+
}

‎hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import static org.apache.hadoop.hbase.HConstants.DEFAULT_HBASE_SPLIT_COORDINATED_BY_ZK;
2121
import static org.apache.hadoop.hbase.HConstants.HBASE_MASTER_LOGCLEANER_PLUGINS;
2222
import static org.apache.hadoop.hbase.HConstants.HBASE_SPLIT_WAL_COORDINATED_BY_ZK;
23-
2423
import com.google.protobuf.Descriptors;
2524
import com.google.protobuf.Service;
2625
import java.io.IOException;
@@ -56,8 +55,10 @@
5655
import javax.servlet.http.HttpServlet;
5756
import javax.servlet.http.HttpServletRequest;
5857
import javax.servlet.http.HttpServletResponse;
58+
import org.apache.commons.io.IOUtils;
5959
import org.apache.commons.lang3.StringUtils;
6060
import org.apache.hadoop.conf.Configuration;
61+
import org.apache.hadoop.fs.FSDataOutputStream;
6162
import org.apache.hadoop.fs.Path;
6263
import org.apache.hadoop.hbase.ChoreService;
6364
import org.apache.hadoop.hbase.ClusterId;
@@ -219,11 +220,9 @@
219220
import org.eclipse.jetty.webapp.WebAppContext;
220221
import org.slf4j.Logger;
221222
import org.slf4j.LoggerFactory;
222-
223223
import org.apache.hbase.thirdparty.com.google.common.annotations.VisibleForTesting;
224224
import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
225225
import org.apache.hbase.thirdparty.com.google.common.collect.Maps;
226-
227226
import org.apache.hadoop.hbase.shaded.protobuf.ProtobufUtil;
228227
import org.apache.hadoop.hbase.shaded.protobuf.RequestConverter;
229228
import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos.GetRegionInfoResponse.CompactionState;
@@ -922,8 +921,15 @@ private void finishActiveMasterInitialization(MonitoredTask status) throws IOExc
922921
// hbck1s against an hbase2 cluster; it could do damage. To skip this behavior, set
923922
// hbase.write.hbck1.lock.file to false.
924923
if (this.conf.getBoolean("hbase.write.hbck1.lock.file", true)) {
925-
HBaseFsck.checkAndMarkRunningHbck(this.conf,
926-
HBaseFsck.createLockRetryCounterFactory(this.conf).create());
924+
Pair<Path, FSDataOutputStream> result = null;
925+
try {
926+
result = HBaseFsck.checkAndMarkRunningHbck(this.conf,
927+
HBaseFsck.createLockRetryCounterFactory(this.conf).create());
928+
} finally {
929+
if (result != null) {
930+
IOUtils.closeQuietly(result.getSecond());
931+
}
932+
}
927933
}
928934

929935
status.setStatus("Initialize ServerManager and schedule SCP for crash servers");

0 commit comments

Comments
 (0)