Repository: hadoop Updated Branches: refs/heads/trunk 8478732bb -> d81cd3611
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/trunk Commit: d81cd3611a449bcd7970ff2f1392a5e868e28f7e Parents: 8478732 Author: Hanisha Koneru <hanishakon...@apache.org> Authored: Wed Aug 8 16:47:25 2018 -0700 Committer: Hanisha Koneru <hanishakon...@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-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-commits-h...@hadoop.apache.org