errose28 commented on code in PR #3741:
URL: https://github.com/apache/ozone/pull/3741#discussion_r980557006
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/volume/HddsVolume.java:
##########
@@ -329,6 +354,169 @@ public void createDbStore(MutableVolumeSet dbVolumeSet)
throws IOException {
}
}
+ // Ensure that volume is initialized properly with
+ // cleanup path. Should disk be re-inserted into
+ // cluster, cleanup path should already be on
+ // disk. This method syncs the HddsVolume
+ // with the path on disk
+ public void checkCleanupDirs(String id) {
+ String tmpPath = createTmpPath(id);
+ String deleteServicePath = tmpPath + TMP_DELETE_SERVICE_DIR;
+ deleteServiceDirPath = Paths.get(deleteServicePath);
+ }
+
+ private String createTmpPath(String id) {
+ StringBuilder stringBuilder = new StringBuilder();
+
+ // HddsVolume root directory path
+ String hddsRoot = getHddsRootDir().toString();
+
+ // HddsVolume path
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ stringBuilder.append(volPath);
+ stringBuilder.append(FILE_SEPARATOR);
+
+ stringBuilder.append(id);
+ stringBuilder.append(TMP_DIR);
+
+ return stringBuilder.toString();
+ }
+
+
+ private void createTmpDir(String id) {
+ tmpDirPath = Paths.get(createTmpPath(id));
+
+ if (Files.notExists(tmpDirPath)) {
+ try {
+ Files.createDirectories(tmpDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", tmpDirPath.toString(), ex);
+ }
+ }
+ }
+
+ private void createDeleteServiceDir() {
+ try {
+ if (tmpDirPath == null) {
+ throw new IOException("tmp directory under volume " +
+ "has not been initialized");
+ }
+ String tmpPath = tmpDirPath.toString();
+ String deleteServicePath = tmpPath + TMP_DELETE_SERVICE_DIR;
+
+ deleteServiceDirPath = Paths.get(deleteServicePath);
+
+ if (Files.notExists(deleteServiceDirPath)) {
+ try {
+ Files.createDirectories(deleteServiceDirPath);
+ } catch (IOException ex) {
+ LOG.error("Error creating {}", deleteServiceDirPath.toString(), ex);
+ }
+ }
+ } catch (IOException ex) {
+ String hddsRoot = getHddsRootDir().toString();
+ String volPath = HddsVolumeUtil.getHddsRoot(hddsRoot);
+
+ LOG.error("Failed to create container_delete_service directory under {}",
+ volPath, ex);
+ }
+ }
+
+ /**
+ * Delete all files under tmp/container_delete_service.
+ */
+ public synchronized void cleanTmpDir() throws IOException {
+ if (getStorageState() != VolumeState.NORMAL) {
+ LOG.debug("Call to clean tmp dir container_delete_service directory "
+ + "for {} while VolumeState {}",
+ getStorageDir(), getStorageState().toString());
+ return;
+ }
+
+ if (!getClusterID().isEmpty()) {
+ checkCleanupDirs(getClusterID());
+ } else {
+ throw new IOException("Volume has no ClusterId");
+ }
+
+ ListIterator<File> leftoversListIt = getDeleteLeftovers();
+
+ while (leftoversListIt.hasNext()) {
+ File file = leftoversListIt.next();
+ try {
+ if (file.isDirectory()) {
+ FileUtils.deleteDirectory(file);
+ } else {
+ FileUtils.delete(file);
+ }
+ } catch (IOException ex) {
+ LOG.error("Failed to delete directory or file inside " +
+ "{}", deleteServiceDirPath.toString(), ex);
+ }
+ }
+ }
+
+ /**
+ * Keep public, in the future might be used to gather metrics
+ * for the files left under tmp/container_delete_service.
+ * @return ListIterator to all the files
+ */
+ public ListIterator<File> getDeleteLeftovers() {
+ List<File> leftovers = new ArrayList<>();
+
+ try {
+ File tmpDir = new File(deleteServiceDirPath.toString());
+
+ for (File file : tmpDir.listFiles()) {
+ leftovers.add(file);
+ }
+ } catch (NullPointerException ex) {
+ LOG.error("tmp directory is null, path doesn't exist", ex);
Review Comment:
`tmpDir.exists()` or an equivalent call is probably a better way to handle
this. Usually we want to avoid catching unchecked exceptions.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/StorageVolumeUtil.java:
##########
@@ -238,6 +238,11 @@ public static boolean checkVolume(StorageVolume volume,
String scmId,
// skipped. Create cluster ID symlink now.
// Else, We are still pre-finalized.
// The existing directory should be left for backwards compatibility.
+ if (volume instanceof HddsVolume) {
+ String id = VersionedDatanodeFeatures.ScmHA.chooseContainerPathID(conf,
+ scmId, clusterId);
+ ((HddsVolume)volume).checkCleanupDirs(id);
+ }
Review Comment:
This will run on fresh clusters or clusters pre-finalized for SCM HA,
because the volume directory has the VERSION file and one ID directory (either
the cluster ID or SCM ID). For clusters that have been upgraded from 1.1.0 the
else block will execute, because there will be 3 files (VERSION file, SCM ID
directory, and symlink to SCM ID directory named after cluster ID). So the else
block needs the cleanup code as well.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/utils/StorageVolumeUtil.java:
##########
@@ -238,6 +238,11 @@ public static boolean checkVolume(StorageVolume volume,
String scmId,
// skipped. Create cluster ID symlink now.
// Else, We are still pre-finalized.
// The existing directory should be left for backwards compatibility.
+ if (volume instanceof HddsVolume) {
+ String id = VersionedDatanodeFeatures.ScmHA.chooseContainerPathID(conf,
+ scmId, clusterId);
+ ((HddsVolume)volume).checkCleanupDirs(id);
+ }
Review Comment:
You can refactor the if/else branching to avoid code duplication when
checking the cleanup directory once this change is made.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ private void deleteInternal(Container container, boolean
force)
DELETE_ON_NON_EMPTY_CONTAINER);
}
}
+ if (container.getContainerData() instanceof KeyValueContainerData) {
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) container.getContainerData();
+ HddsVolume hddsVolume = keyValueContainerData.getVolume();
+
+ if (hddsVolume.getClusterID() != null) {
Review Comment:
Why do we need this check?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ private void deleteInternal(Container container, boolean
force)
DELETE_ON_NON_EMPTY_CONTAINER);
}
}
+ if (container.getContainerData() instanceof KeyValueContainerData) {
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) container.getContainerData();
+ HddsVolume hddsVolume = keyValueContainerData.getVolume();
+
+ if (hddsVolume.getClusterID() != null) {
+ try {
+ // Rename container location
+ boolean success = hddsVolume
+ .moveToTmpDeleteDirectory(keyValueContainerData);
+
+ if (success) {
Review Comment:
If this fails we need to throw an exception to prevent further steps from
executing.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1135,6 +1136,31 @@ private void deleteInternal(Container container, boolean
force)
DELETE_ON_NON_EMPTY_CONTAINER);
}
}
+ if (container.getContainerData() instanceof KeyValueContainerData) {
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) container.getContainerData();
+ HddsVolume hddsVolume = keyValueContainerData.getVolume();
+
+ if (hddsVolume.getClusterID() != null) {
+ try {
+ // Rename container location
+ boolean success = hddsVolume
+ .moveToTmpDeleteDirectory(keyValueContainerData);
+
+ if (success) {
+ String containerPath = keyValueContainerData
+ .getContainerPath().toString();
+ File containerDir = new File(containerPath);
+
+ LOG.info("Container {} has been successfuly moved under {}",
+ containerDir.getName(),
hddsVolume.getDeleteServiceDirPath());
+ }
+ } catch (IOException ex) {
Review Comment:
Same idea as above, we want to let this exception propagate to abort the
delete process. Logging on error might be better done inside the method that is
doing the move. Right now it is not clear which methods in the try/catch block
throw the exception and why that means the volume is not initialized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
