You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-commits@hadoop.apache.org by su...@apache.org on 2018/08/10 23:32:02 UTC
[02/25] hadoop git commit: HDDS-267. Handle consistency issues during
container update/close.
HDDS-267. Handle consistency issues during container update/close.
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo
Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/d81cd361
Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/d81cd361
Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/d81cd361
Branch: refs/heads/HDFS-12943
Commit: d81cd3611a449bcd7970ff2f1392a5e868e28f7e
Parents: 8478732
Author: Hanisha Koneru <ha...@apache.org>
Authored: Wed Aug 8 16:47:25 2018 -0700
Committer: Hanisha Koneru <ha...@apache.org>
Committed: Wed Aug 8 16:47:25 2018 -0700
----------------------------------------------------------------------
.../container/common/impl/ContainerData.java | 1 -
.../container/keyvalue/KeyValueContainer.java | 54 ++++++-------------
.../container/keyvalue/KeyValueHandler.java | 21 ++++++--
.../keyvalue/TestKeyValueContainer.java | 16 ------
.../container/keyvalue/TestKeyValueHandler.java | 55 ++++++++++++++++----
.../common/impl/TestContainerPersistence.java | 8 ---
6 files changed, 80 insertions(+), 75 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hadoop/blob/d81cd361/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
----------------------------------------------------------------------
diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
index 5803628..26954a7 100644
--- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
+++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java
@@ -257,7 +257,6 @@ public abstract class ContainerData {
* Marks this container as closed.
*/
public synchronized void closeContainer() {
- // TODO: closed or closing here
setState(ContainerLifeCycleState.CLOSED);
}
http://git-wip-us.apache.org/repos/asf/hadoop/blob/d81cd361/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
----------------------------------------------------------------------
diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
index 353fe4f..c96f997 100644
--- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
+++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueContainer.java
@@ -138,7 +138,7 @@ public class KeyValueContainer implements Container {
// Create .container file
File containerFile = getContainerFile();
- writeToContainerFile(containerFile, true);
+ createContainerFile(containerFile);
} catch (StorageContainerException ex) {
if (containerMetaDataPath != null && containerMetaDataPath.getParentFile()
@@ -165,11 +165,11 @@ public class KeyValueContainer implements Container {
}
/**
- * Creates .container file and checksum file.
+ * Writes to .container file.
*
- * @param containerFile
- * @param isCreate true if we are creating a new container file and false if
- * we are updating an existing container file.
+ * @param containerFile container file name
+ * @param isCreate True if creating a new file. False is updating an
+ * existing container file.
* @throws StorageContainerException
*/
private void writeToContainerFile(File containerFile, boolean isCreate)
@@ -181,19 +181,18 @@ public class KeyValueContainer implements Container {
ContainerDataYaml.createContainerFile(
ContainerType.KeyValueContainer, containerData, tempContainerFile);
+ // NativeIO.renameTo is an atomic function. But it might fail if the
+ // container file already exists. Hence, we handle the two cases
+ // separately.
if (isCreate) {
- // When creating a new container, .container file should not exist
- // already.
NativeIO.renameTo(tempContainerFile, containerFile);
} else {
- // When updating a container, the .container file should exist. If
- // not, the container is in an inconsistent state.
Files.move(tempContainerFile.toPath(), containerFile.toPath(),
StandardCopyOption.REPLACE_EXISTING);
}
} catch (IOException ex) {
- throw new StorageContainerException("Error during creation of " +
+ throw new StorageContainerException("Error while creating/ updating " +
".container file. ContainerID: " + containerId, ex,
CONTAINER_FILES_CREATE_ERROR);
} finally {
@@ -206,27 +205,14 @@ public class KeyValueContainer implements Container {
}
}
+ private void createContainerFile(File containerFile)
+ throws StorageContainerException {
+ writeToContainerFile(containerFile, true);
+ }
private void updateContainerFile(File containerFile)
throws StorageContainerException {
-
- long containerId = containerData.getContainerID();
-
- if (!containerFile.exists()) {
- throw new StorageContainerException("Container is an Inconsistent " +
- "state, missing .container file. ContainerID: " + containerId,
- INVALID_CONTAINER_STATE);
- }
-
- try {
- writeToContainerFile(containerFile, false);
- } catch (IOException e) {
- //TODO : Container update failure is not handled currently. Might
- // lead to loss of .container file. When Update container feature
- // support is added, this failure should also be handled.
- throw new StorageContainerException("Container update failed. " +
- "ContainerID: " + containerId, CONTAINER_FILES_CREATE_ERROR);
- }
+ writeToContainerFile(containerFile, false);
}
@@ -256,19 +242,15 @@ public class KeyValueContainer implements Container {
// complete this action
try {
writeLock();
- long containerId = containerData.getContainerID();
- if(!containerData.isValid()) {
- LOG.debug("Invalid container data. Container Id: {}", containerId);
- throw new StorageContainerException("Invalid container data. " +
- "ContainerID: " + containerId, INVALID_CONTAINER_STATE);
- }
+
containerData.closeContainer();
File containerFile = getContainerFile();
-
// update the new container data to .container File
updateContainerFile(containerFile);
} catch (StorageContainerException ex) {
+ // Failed to update .container file. Reset the state to CLOSING
+ containerData.setState(ContainerLifeCycleState.CLOSING);
throw ex;
} finally {
writeUnlock();
@@ -332,8 +314,6 @@ public class KeyValueContainer implements Container {
// update the new container data to .container File
updateContainerFile(containerFile);
} catch (StorageContainerException ex) {
- // TODO:
- // On error, reset the metadata.
containerData.setMetadata(oldMetadata);
throw ex;
} finally {
http://git-wip-us.apache.org/repos/asf/hadoop/blob/d81cd361/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
----------------------------------------------------------------------
diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
index a281a53..f4699dd 100644
--- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
+++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java
@@ -29,6 +29,8 @@ import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
.ContainerCommandResponseProto;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
+ .ContainerLifeCycleState;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
.ContainerType;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
.GetSmallFileRequestProto;
@@ -77,6 +79,8 @@ import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;
import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
+ .Result.CLOSED_CONTAINER_RETRY;
+import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
.Result.CONTAINER_INTERNAL_ERROR;
import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
.Result.CLOSED_CONTAINER_IO;
@@ -378,8 +382,18 @@ public class KeyValueHandler extends Handler {
return ContainerUtils.malformedRequest(request);
}
+ long containerID = kvContainer.getContainerData().getContainerID();
+ ContainerLifeCycleState containerState = kvContainer.getContainerState();
+
try {
- checkContainerOpen(kvContainer);
+ if (containerState == ContainerLifeCycleState.CLOSED) {
+ throw new StorageContainerException("Container already closed. " +
+ "ContainerID: " + containerID, CLOSED_CONTAINER_RETRY);
+ } else if (containerState == ContainerLifeCycleState.INVALID) {
+ LOG.debug("Invalid container data. ContainerID: {}", containerID);
+ throw new StorageContainerException("Invalid container data. " +
+ "ContainerID: " + containerID, INVALID_CONTAINER_STATE);
+ }
KeyValueContainerData kvData = kvContainer.getContainerData();
@@ -773,10 +787,9 @@ public class KeyValueHandler extends Handler {
private void checkContainerOpen(KeyValueContainer kvContainer)
throws StorageContainerException {
- ContainerProtos.ContainerLifeCycleState containerState =
- kvContainer.getContainerState();
+ ContainerLifeCycleState containerState = kvContainer.getContainerState();
- if (containerState == ContainerProtos.ContainerLifeCycleState.OPEN) {
+ if (containerState == ContainerLifeCycleState.OPEN) {
return;
} else {
String msg = "Requested operation not allowed as ContainerState is " +
http://git-wip-us.apache.org/repos/asf/hadoop/blob/d81cd361/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java
----------------------------------------------------------------------
diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java
index 37c7f8a..6ff2eca 100644
--- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java
+++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java
@@ -33,7 +33,6 @@ import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
import org.apache.hadoop.ozone.container.common.volume
.RoundRobinVolumeChoosingPolicy;
import org.apache.hadoop.ozone.container.common.volume.VolumeSet;
-
import org.apache.hadoop.ozone.container.keyvalue.helpers.KeyUtils;
import org.apache.hadoop.test.GenericTestUtils;
import org.apache.hadoop.util.DiskChecker;
@@ -243,21 +242,6 @@ public class TestKeyValueContainer {
}
@Test
- public void testCloseInvalidContainer() throws Exception {
- try {
- keyValueContainerData.setState(ContainerProtos.ContainerLifeCycleState
- .INVALID);
- keyValueContainer.create(volumeSet, volumeChoosingPolicy, scmId);
- keyValueContainer.close();
- fail("testCloseInvalidContainer failed");
- } catch (StorageContainerException ex) {
- assertEquals(ContainerProtos.Result.INVALID_CONTAINER_STATE,
- ex.getResult());
- GenericTestUtils.assertExceptionContains("Invalid container data", ex);
- }
- }
-
- @Test
public void testUpdateContainer() throws IOException {
keyValueContainer.create(volumeSet, volumeChoosingPolicy, scmId);
Map<String, String> metadata = new HashMap<>();
http://git-wip-us.apache.org/repos/asf/hadoop/blob/d81cd361/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
----------------------------------------------------------------------
diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
index 747687b..ce12e1f 100644
--- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
+++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java
@@ -25,12 +25,16 @@ import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos
.ContainerCommandRequestProto;
+import org.apache.hadoop.hdds.scm.container.common.helpers
+ .StorageContainerException;
import org.apache.hadoop.hdfs.MiniDFSCluster;
import org.apache.hadoop.ozone.container.common.helpers.ContainerMetrics;
import org.apache.hadoop.ozone.container.common.impl.ContainerSet;
import org.apache.hadoop.ozone.container.common.impl.HddsDispatcher;
import org.apache.hadoop.ozone.container.common.volume.VolumeSet;
import org.apache.hadoop.test.GenericTestUtils;
+import org.junit.Assert;
+import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestRule;
@@ -59,8 +63,8 @@ public class TestKeyValueHandler {
@Rule
public TestRule timeout = new Timeout(300000);
- private HddsDispatcher dispatcher;
- private KeyValueHandler handler;
+ private static HddsDispatcher dispatcher;
+ private static KeyValueHandler handler;
private final static String DATANODE_UUID = UUID.randomUUID().toString();
@@ -69,14 +73,11 @@ public class TestKeyValueHandler {
private static final long DUMMY_CONTAINER_ID = 9999;
- @Test
- /**
- * Test that Handler handles different command types correctly.
- */
- public void testHandlerCommandHandling() throws Exception{
+ @BeforeClass
+ public static void setup() throws StorageContainerException {
// Create mock HddsDispatcher and KeyValueHandler.
- this.handler = Mockito.mock(KeyValueHandler.class);
- this.dispatcher = Mockito.mock(HddsDispatcher.class);
+ handler = Mockito.mock(KeyValueHandler.class);
+ dispatcher = Mockito.mock(HddsDispatcher.class);
Mockito.when(dispatcher.getHandler(any())).thenReturn(handler);
Mockito.when(dispatcher.dispatch(any())).thenCallRealMethod();
Mockito.when(dispatcher.getContainer(anyLong())).thenReturn(
@@ -84,6 +85,13 @@ public class TestKeyValueHandler {
Mockito.when(handler.handle(any(), any())).thenCallRealMethod();
doCallRealMethod().when(dispatcher).setMetricsForTesting(any());
dispatcher.setMetricsForTesting(Mockito.mock(ContainerMetrics.class));
+ }
+
+ @Test
+ /**
+ * Test that Handler handles different command types correctly.
+ */
+ public void testHandlerCommandHandling() throws Exception {
// Test Create Container Request handling
ContainerCommandRequestProto createContainerRequest =
@@ -250,4 +258,33 @@ public class TestKeyValueHandler {
}
+ @Test
+ public void testCloseInvalidContainer() {
+ long containerID = 1234L;
+ Configuration conf = new Configuration();
+ KeyValueContainerData kvData = new KeyValueContainerData(containerID, 1);
+ KeyValueContainer container = new KeyValueContainer(kvData, conf);
+ kvData.setState(ContainerProtos.ContainerLifeCycleState.INVALID);
+
+ // Create Close container request
+ ContainerCommandRequestProto closeContainerRequest =
+ ContainerProtos.ContainerCommandRequestProto.newBuilder()
+ .setCmdType(ContainerProtos.Type.CloseContainer)
+ .setContainerID(DUMMY_CONTAINER_ID)
+ .setDatanodeUuid(DATANODE_UUID)
+ .setCloseContainer(ContainerProtos.CloseContainerRequestProto
+ .getDefaultInstance())
+ .build();
+ dispatcher.dispatch(closeContainerRequest);
+
+ Mockito.when(handler.handleCloseContainer(any(), any()))
+ .thenCallRealMethod();
+ // Closing invalid container should return error response.
+ ContainerProtos.ContainerCommandResponseProto response =
+ handler.handleCloseContainer(closeContainerRequest, container);
+
+ Assert.assertTrue("Close container should return Invalid container error",
+ response.getResult().equals(
+ ContainerProtos.Result.INVALID_CONTAINER_STATE));
+ }
}
http://git-wip-us.apache.org/repos/asf/hadoop/blob/d81cd361/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java
----------------------------------------------------------------------
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java
index 5322c8e..016b94c 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/container/common/impl/TestContainerPersistence.java
@@ -775,14 +775,6 @@ public class TestContainerPersistence {
Assert.assertEquals("bilbo_new_1",
actualNewData.getMetadata().get("owner"));
- // Update a non-existing container
- exception.expect(StorageContainerException.class);
- exception.expectMessage("Container is an Inconsistent " +
- "state, missing .container file.");
- Container nonExistentContainer = new KeyValueContainer(
- new KeyValueContainerData(RandomUtils.nextLong(),
- ContainerTestHelper.CONTAINER_MAX_SIZE_GB), conf);
- nonExistentContainer.update(newMetadata, false);
}
private KeyData writeKeyHelper(BlockID blockID)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-commits-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-commits-help@hadoop.apache.org