Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,8 @@
* Provides some very generic helpers which might be used across the tests.
*/
public abstract class GenericTestUtils {
public static final String SYSPROP_TEST_DATA_DIR = "test.build.data";
public static final String DEFAULT_TEST_DATA_DIR;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems DEFAULT_TEST_DATA_DIR can be removed as well, it's not used anymore

public static final String DEFAULT_TEST_DATA_PATH = "target/test/data/";

/**
* Error string used in
* {@link GenericTestUtils#waitFor(BooleanSupplier, int, int)}.
Expand Down Expand Up @@ -90,31 +89,6 @@ public static Instant getTestStartTime() {
return Instant.ofEpochMilli(System.currentTimeMillis());
}

/**
* Get a temp path. This may or may not be relative; it depends on what the
* {@link #SYSPROP_TEST_DATA_DIR} is set to. If unset, it returns a path
* under the relative path {@link #DEFAULT_TEST_DATA_PATH}
*
* @param subpath sub path, with no leading "/" character
* @return a string to use in paths
*
* @deprecated use {@link org.junit.jupiter.api.io.TempDir} instead.
*/
@Deprecated
public static String getTempPath(String subpath) {
String prop = WINDOWS ? DEFAULT_TEST_DATA_PATH
: System.getProperty(SYSPROP_TEST_DATA_DIR, DEFAULT_TEST_DATA_PATH);

if (prop.isEmpty()) {
// corner case: property is there but empty
prop = DEFAULT_TEST_DATA_PATH;
}
if (!prop.endsWith("/")) {
prop = prop + "/";
}
return prop + subpath;
}

/**
* Wait for the specified test to return true. The test will be performed
* initially and then every {@code checkEveryMillis} until at least
Expand All @@ -133,7 +107,7 @@ public static String getTempPath(String subpath) {
* @throws InterruptedException if the method is interrupted while waiting
*/
public static void waitFor(BooleanSupplier check, int checkEveryMillis,
int waitForMillis) throws TimeoutException, InterruptedException {
int waitForMillis) throws TimeoutException, InterruptedException {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's do not format changes that are not related to the jira ticket

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In addition to being unrelated, this specific formatting should be avoided. Whenever visibility / return type / method name / other modifiers are changed, we would have to reindent all parameters.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Yes, I'll revert the format changes and take into account that they should be avoided.

Preconditions.checkNotNull(check, ERROR_MISSING_ARGUMENT);
Preconditions.checkArgument(waitForMillis >= checkEveryMillis,
ERROR_INVALID_ARGUMENT);
Expand Down Expand Up @@ -176,7 +150,7 @@ public static void setLogLevel(Logger logger, Level level) {
}

public static void setLogLevel(org.slf4j.Logger logger,
org.slf4j.event.Level level) {
org.slf4j.event.Level level) {
setLogLevel(toLog4j(logger), Level.toLevel(level.toString()));
}

Expand All @@ -197,7 +171,7 @@ public static void withLogDisabled(Class<?> clazz, Runnable task) {
}

public static <T> T mockFieldReflection(Object object, String fieldName)
throws NoSuchFieldException, IllegalAccessException {
throws NoSuchFieldException, IllegalAccessException {
Field field = object.getClass().getDeclaredField(fieldName);
boolean isAccessible = field.isAccessible();

Expand All @@ -217,7 +191,7 @@ public static <T> T mockFieldReflection(Object object, String fieldName)
}

public static <T> T getFieldReflection(Object object, String fieldName)
throws NoSuchFieldException, IllegalAccessException {
throws NoSuchFieldException, IllegalAccessException {
Field field = object.getClass().getDeclaredField(fieldName);
boolean isAccessible = field.isAccessible();

Expand All @@ -237,7 +211,7 @@ public static <T> T getFieldReflection(Object object, String fieldName)
public static <K, V> Map<V, K> getReverseMap(Map<K, List<V>> map) {
return map.entrySet().stream().flatMap(entry -> entry.getValue().stream()
.map(v -> Pair.of(v, entry.getKey())))
.collect(Collectors.toMap(Pair::getKey, Pair::getValue));
.collect(Collectors.toMap(Pair::getKey, Pair::getValue));
}

/**
Expand Down Expand Up @@ -281,6 +255,7 @@ public void clearOutput() {
writer().getBuffer().setLength(0);
}
}

@Deprecated
public static Logger toLog4j(org.slf4j.Logger logger) {
return LogManager.getLogger(logger.getName());
Expand All @@ -298,7 +273,9 @@ public static PrintStreamCapturer captureErr() {
return new SystemErrCapturer();
}

/** Capture contents of a {@code PrintStream}, until {@code close()}d. */
/**
* Capture contents of a {@code PrintStream}, until {@code close()}d.
*/
public abstract static class PrintStreamCapturer implements AutoCloseable, Supplier<String> {
private final ByteArrayOutputStream bytes;
private final PrintStream bytesPrintStream;
Expand Down Expand Up @@ -377,6 +354,7 @@ public SystemOutCapturer() {

/**
* Replaces {@link System#in} with a stream that provides {@code lines} as input.
*
* @return an {@code AutoCloseable} to restore the original {@link System#in} stream
*/
public static AutoCloseable supplyOnSystemIn(String... lines) {
Expand Down Expand Up @@ -456,9 +434,10 @@ public static final class ReflectionUtils {
/**
* This method provides the modifiers field using reflection approach which is compatible
* for both pre Java 9 and post java 9 versions.
*
* @return modifiers field
* @throws IllegalAccessException illegalAccessException,
* @throws NoSuchFieldException noSuchFieldException.
* @throws NoSuchFieldException noSuchFieldException.
*/
public static Field getModifiersField() throws IllegalAccessException, NoSuchFieldException {
Field modifiersField = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import org.apache.hadoop.ozone.container.common.statemachine.DatanodeStateMachine;
import org.apache.hadoop.ozone.container.common.statemachine.EndpointStateMachine;
import org.apache.hadoop.ozone.container.common.volume.StorageVolume;
import org.apache.ozone.test.GenericTestUtils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -203,9 +202,11 @@ public void testKeepPortsWhenRestartDN() throws Exception {

/**
* Test that a DN can register with SCM even if it was started before the SCM.
*
* @throws Exception
*/
@Test @Timeout(100)
@Test
@Timeout(100)
public void testDNstartAfterSCM() throws Exception {
// Start a cluster with 3 DN
cluster = MiniOzoneCluster.newBuilder(conf)
Expand Down Expand Up @@ -248,9 +249,11 @@ public void testDNstartAfterSCM() throws Exception {

/**
* Test that multiple datanode directories are created in MiniOzoneCluster.
*
* @throws Exception
*/
@Test @Timeout(60)
@Test
@Timeout(60)
public void testMultipleDataDirs() throws Exception {
// Start a cluster with 3 DN and configure reserved space in each DN
String reservedSpace = "1B";
Expand All @@ -267,7 +270,7 @@ public void testMultipleDataDirs() throws Exception {
+ "-" + cluster.getClusterId();
assertEquals(name, cluster.getName());

final String baseDir = GenericTestUtils.getTempPath(name);
final String baseDir = MiniOzoneCluster.getTempPath(name);
assertEquals(baseDir, cluster.getBaseDir());


Expand All @@ -278,8 +281,8 @@ public void testMultipleDataDirs() throws Exception {
assertEquals(3, volumeList.size());

volumeList.forEach(storageVolume -> assertEquals(
(long) StorageSize.parse(reservedSpace).getValue(),
storageVolume.getVolumeUsage().get().getReservedInBytes()));
(long) StorageSize.parse(reservedSpace).getValue(),
storageVolume.getVolumeUsage().get().getReservedInBytes()));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -35,20 +35,22 @@
import org.apache.hadoop.ozone.om.OzoneManager;
import org.apache.hadoop.ozone.recon.ReconServer;
import org.apache.hadoop.security.authentication.client.AuthenticationException;
import org.apache.ozone.test.GenericTestUtils;
import org.apache.ratis.util.ExitUtils;
import org.apache.ratis.util.function.CheckedFunction;

/**
* Interface used for MiniOzoneClusters.
*/
public interface MiniOzoneCluster extends AutoCloseable {
String SYSPROP_TEST_DATA_DIR = "test.build.data";
String DEFAULT_TEST_DATA_PATH = "target/test/data/";

boolean WINDOWS = System.getProperty("os.name").startsWith("Windows");

/**
* Returns the Builder to construct MiniOzoneCluster.
*
* @param conf OzoneConfiguration
*
* @return MiniOzoneCluster builder
*/
static Builder newBuilder(OzoneConfiguration conf) {
Expand All @@ -59,7 +61,6 @@ static Builder newBuilder(OzoneConfiguration conf) {
* Returns the Builder to construct MiniOzoneHACluster.
*
* @param conf OzoneConfiguration
*
* @return MiniOzoneCluster builder
*/
static MiniOzoneHAClusterImpl.Builder newHABuilder(OzoneConfiguration conf) {
Expand All @@ -78,7 +79,7 @@ static MiniOzoneHAClusterImpl.Builder newHABuilder(OzoneConfiguration conf) {
* configured {@link HddsDatanodeService} registers with
* {@link StorageContainerManager}.
*
* @throws TimeoutException In case of timeout
* @throws TimeoutException In case of timeout
* @throws InterruptedException In case of interrupt while waiting
*/
void waitForClusterToBeReady() throws TimeoutException, InterruptedException;
Expand All @@ -87,14 +88,14 @@ static MiniOzoneHAClusterImpl.Builder newHABuilder(OzoneConfiguration conf) {
* Waits for at least one RATIS pipeline of given factor to be reported in open
* state.
*
* @param factor replication factor
* @param factor replication factor
* @param timeoutInMs timeout value in milliseconds
* @throws TimeoutException In case of timeout
* @throws TimeoutException In case of timeout
* @throws InterruptedException In case of interrupt while waiting
*/
void waitForPipelineTobeReady(HddsProtos.ReplicationFactor factor,
int timeoutInMs)
throws TimeoutException, InterruptedException;
throws TimeoutException, InterruptedException;

/**
* Sets the timeout value after which
Expand All @@ -107,7 +108,7 @@ void waitForPipelineTobeReady(HddsProtos.ReplicationFactor factor,
/**
* Waits/blocks till the cluster is out of safe mode.
*
* @throws TimeoutException TimeoutException In case of timeout
* @throws TimeoutException TimeoutException In case of timeout
* @throws InterruptedException In case of interrupt while waiting
*/
void waitTobeOutOfSafeMode() throws TimeoutException, InterruptedException;
Expand Down Expand Up @@ -194,6 +195,7 @@ void restartHddsDatanode(int i, boolean waitForDatanode)
*/
void restartHddsDatanode(DatanodeDetails dn, boolean waitForDatanode)
throws InterruptedException, TimeoutException, IOException;

/**
* Shutdown a particular HddsDatanode.
*
Expand Down Expand Up @@ -250,7 +252,29 @@ default String getName() {
}

default String getBaseDir() {
return GenericTestUtils.getTempPath(getName());
return getTempPath(getName());
}

/**
* Get a temp path. This may or may not be relative; it depends on what the
* {@link #SYSPROP_TEST_DATA_DIR} is set to. If unset, it returns a path
* under the relative path {@link #DEFAULT_TEST_DATA_PATH}
*
* @param subpath sub path, with no leading "/" character
* @return a string to use in paths
*/
static String getTempPath(String subpath) {
String prop = WINDOWS ? DEFAULT_TEST_DATA_PATH
: System.getProperty(SYSPROP_TEST_DATA_DIR, DEFAULT_TEST_DATA_PATH);

if (prop.isEmpty()) {
// corner case: property is there but empty
prop = DEFAULT_TEST_DATA_PATH;
}
if (!prop.endsWith("/")) {
prop = prop + "/";
}
return prop + subpath;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suggest moving the method as protected and related constants as private to MiniOzoneCluster.Builder for a cleaner MiniOzoneCluster interface.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the suggestion


/**
Expand All @@ -275,7 +299,7 @@ abstract class Builder {
protected boolean includeRecon = false;

protected int numOfDatanodes = 3;
protected boolean startDataNodes = true;
protected boolean startDataNodes = true;
protected CertificateClient certClient;
protected SecretKeyClient secretKeyClient;
protected DatanodeFactory dnFactory = UniformDatanodesFactory.newBuilder().build();
Expand All @@ -289,8 +313,10 @@ protected Builder(OzoneConfiguration conf) {
ExitUtils.disableSystemExit();
}

/** Prepare the builder for another call to {@link #build()}, avoiding conflict
* between the clusters created. */
/**
* Prepare the builder for another call to {@link #build()}, avoiding conflict
* between the clusters created.
*/
protected void prepareForNextBuild() {
conf = new OzoneConfiguration(conf);
setClusterId();
Expand All @@ -303,8 +329,7 @@ public Builder setSCMConfigurator(SCMConfigurator configurator) {

private void setClusterId() {
clusterId = UUID.randomUUID().toString();
path = GenericTestUtils.getTempPath(
MiniOzoneClusterImpl.class.getSimpleName() + "-" + clusterId);
path = getTempPath(MiniOzoneClusterImpl.class.getSimpleName() + "-" + clusterId);
}

/**
Expand Down Expand Up @@ -338,7 +363,6 @@ public Builder setSecretKeyClient(SecretKeyClient client) {
* MiniOzoneCluster.
*
* @param val number of datanodes
*
* @return MiniOzoneCluster.Builder
*/
public Builder setNumDatanodes(int val) {
Expand Down Expand Up @@ -380,9 +404,12 @@ interface DatanodeFactory extends CheckedFunction<OzoneConfiguration, OzoneConfi
// marker
}

/** Service to manage as part of the mini cluster. */
/**
* Service to manage as part of the mini cluster.
*/
interface Service {
void start(OzoneConfiguration conf) throws Exception;

void stop() throws Exception;
}
}