chihsuan commented on code in PR #11291:
URL: https://github.com/apache/ozone/pull/11291#discussion_r4100961887
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingService.java:
##########
@@ -258,6 +270,157 @@ void testUpdateAndRestart() throws Exception {
.isEqualTo(newInterval.toMillis());
}
+ @ParameterizedTest
+ @CsvSource({"1, true, false", "2, true, false", "3, true, false",
+ "1, false, false", "2, false, false", "3, false, false",
+ "1, true, true", "2, true, true", "3, true, true",
+ "1, false, true", "2, false, true", "3, false, true"})
+ void testPurgeDirectoriesSubmitFailure(int failedBatch, boolean
throwException, boolean snapshot) throws Exception {
+ List<PurgePathRequest> purgeList = new ArrayList<>();
+ String directoryName = StringUtils.repeat("d", 1200);
+ OzoneManagerProtocolProtos.KeyInfo keyInfo =
+ OMRequestTestUtils.createOmKeyInfo("volume", "bucket", "key",
RatisReplicationConfig.getInstance(ONE))
+ .build().getProtobuf(ClientVersion.CURRENT_VERSION);
+ for (int i = 0; i < 3; i++) {
+ purgeList.add(PurgePathRequest.newBuilder().setVolumeId(1).setBucketId(2)
+ .setDeletedDir(directoryName + i).addMarkDeletedSubDirs(keyInfo)
+ .addDeletedSubFiles(keyInfo).addDeletedSubFiles(keyInfo).build());
+ }
+
+ OzoneConfiguration conf = createConfAndInitValues(1);
+ // Each path fits in one batch, but two paths exceed the byte limit.
+
conf.setStorageSize(OMConfigKeys.OZONE_OM_RATIS_LOG_APPENDER_QUEUE_BYTE_LIMIT,
2304, StorageUnit.BYTES);
+ OmTestManagers managers = new OmTestManagers(conf);
+ try {
+ KeyManager keyManager = managers.getKeyManager();
+ DirectoryDeletingService service = keyManager.getDirDeletingService();
+ service.suspend();
+ DirectoryDeletingService subject = Mockito.spy(service);
+ DeletingServiceMetrics metrics = Mockito.spy(subject.getMetrics());
+ Mockito.doReturn(metrics).when(subject).getMetrics();
+ long initialDirs = metrics.getNumDirsSentForPurge();
+ long initialSubDirs = metrics.getNumSubDirsSentForPurge();
+ long initialSubFiles = metrics.getNumSubFilesSentForPurge();
+ String snapshotKey = snapshot ? "snapshot" : null;
+ subject.getTasks();
+
+ OMResponse success =
OMResponse.newBuilder().setCmdType(OzoneManagerProtocolProtos.Type.PurgeDirectories)
+
.setStatus(OzoneManagerProtocolProtos.Status.OK).setSuccess(true).build();
+ List<OMRequest> submitted = new ArrayList<>();
+ Mockito.doAnswer(invocation -> {
+ submitted.add(invocation.getArgument(0));
+ if (submitted.size() == failedBatch) {
+ if (throwException) {
+ throw new ServiceException("Transient purge submit failure");
+ }
+ return
success.toBuilder().setStatus(OzoneManagerProtocolProtos.Status.INTERNAL_ERROR)
+ .setSuccess(false).build();
+ }
+ return success;
+ }).when(subject).submitRequest(Mockito.any(OMRequest.class));
+
+ subject.optimizeDirDeletesAndSubmitRequest(Collections.emptyList(),
purgeList, snapshotKey,
+ Time.monotonicNow(), keyManager, kv -> true, kv -> true,
Collections.emptyMap(), null, 1,
+ new AtomicInteger(0));
+
+ assertThat(submitted).hasSize(failedBatch);
+ int successfulBatches = failedBatch - 1;
+ assertThat(subject.getDeletedDirsCount()).isEqualTo(successfulBatches);
+ assertThat(subject.getMovedDirsCount()).isEqualTo(successfulBatches);
+ assertThat(subject.getMovedFilesCount()).isEqualTo(2L *
successfulBatches);
+ assertThat(metrics.getNumDirsSentForPurge() -
initialDirs).isEqualTo(successfulBatches);
+ assertThat(metrics.getNumSubDirsSentForPurge() -
initialSubDirs).isEqualTo(successfulBatches);
+ assertThat(metrics.getNumSubFilesSentForPurge() -
initialSubFiles).isEqualTo(2L * successfulBatches);
+ subject.execTaskCompletion();
+ Mockito.verify(metrics).updateAosDdsLastRunMetrics(snapshot ? 0 :
successfulBatches,
+ snapshot ? 0 : successfulBatches, snapshot ? 0 : 2L *
successfulBatches);
+ Mockito.verify(metrics).updateSnapDdsLastRunMetrics(snapshot ?
successfulBatches : 0,
+ snapshot ? successfulBatches : 0, snapshot ? 2L * successfulBatches
: 0);
+
+ // Successfully purged paths are no longer pending on the next run.
+ List<PurgePathRequest> remaining = new
ArrayList<>(purgeList.subList(successfulBatches, purgeList.size()));
+ Mockito.clearInvocations(metrics);
+ subject.getTasks();
+ subject.optimizeDirDeletesAndSubmitRequest(Collections.emptyList(),
remaining, snapshotKey,
+ Time.monotonicNow(), keyManager, kv -> true, kv -> true,
Collections.emptyMap(), null, 2,
+ new AtomicInteger(0));
+
+ assertThat(submitted).hasSize(failedBatch + remaining.size());
+ for (int i = 0; i < remaining.size(); i++) {
+ OMRequest request = submitted.get(failedBatch + i);
+
assertThat(request.getCmdType()).isEqualTo(OzoneManagerProtocolProtos.Type.PurgeDirectories);
+
assertThat(request.getPurgeDirectoriesRequest().getDeletedPathList()).containsExactly(remaining.get(i));
+ }
+ assertThat(subject.getDeletedDirsCount()).isEqualTo(3);
+ assertThat(subject.getMovedDirsCount()).isEqualTo(3);
+ assertThat(subject.getMovedFilesCount()).isEqualTo(6);
+ assertThat(metrics.getNumDirsSentForPurge() - initialDirs).isEqualTo(3);
+ assertThat(metrics.getNumSubDirsSentForPurge() -
initialSubDirs).isEqualTo(3);
+ assertThat(metrics.getNumSubFilesSentForPurge() -
initialSubFiles).isEqualTo(6);
+ subject.execTaskCompletion();
+ Mockito.verify(metrics).updateAosDdsLastRunMetrics(snapshot ? 0 :
remaining.size(),
+ snapshot ? 0 : remaining.size(), snapshot ? 0 : 2L *
remaining.size());
+ Mockito.verify(metrics).updateSnapDdsLastRunMetrics(snapshot ?
remaining.size() : 0,
+ snapshot ? remaining.size() : 0, snapshot ? 2L * remaining.size() :
0);
+ } finally {
+ managers.stop();
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(ints = {0, 1, 2, 3})
+ void testPurgeDirectoriesRecursiveAccounting(int failedBatch) throws
Exception {
+ OzoneConfiguration conf = createConfAndInitValues(1);
+
conf.setStorageSize(OMConfigKeys.OZONE_OM_RATIS_LOG_APPENDER_QUEUE_BYTE_LIMIT,
2304, StorageUnit.BYTES);
+ OmTestManagers managers = new OmTestManagers(conf);
+ try {
+ KeyManager keyManager = managers.getKeyManager();
+ DirectoryDeletingService service = keyManager.getDirDeletingService();
+ service.suspend();
+ DirectoryDeletingService subject = Mockito.spy(service);
+ OMMetadataManager metadataManager = managers.getMetadataManager();
+ OmKeyInfo child = OMRequestTestUtils.createOmKeyInfo("volume", "bucket",
StringUtils.repeat("d", 1200),
+
RatisReplicationConfig.getInstance(ONE)).setObjectID(3).setParentObjectID(2).build();
+ String childDeleteKey =
metadataManager.getOzoneDeletePathKey(child.getObjectID(),
+ metadataManager.getOzonePathKey(1, 2, child.getParentObjectID(),
child.getFileName()));
+ List<Pair<String, OmKeyInfo>> subDirs = new ArrayList<>();
+ subDirs.add(Pair.of(childDeleteKey, child));
+ PurgePathRequest parent =
PurgePathRequest.newBuilder().setVolumeId(1).setBucketId(2)
+
.setDeletedDir("parent").addMarkDeletedSubDirs(child.getProtobuf(ClientVersion.CURRENT_VERSION)).build();
+ List<PurgePathRequest> purgeList = new ArrayList<>();
+ purgeList.add(parent);
+ PurgePathRequest otherBucket =
PurgePathRequest.newBuilder().setVolumeId(1).setBucketId(3)
+ .setDeletedDir("other-parent").build();
+ purgeList.add(otherBucket);
+ List<OMRequest> submitted = new ArrayList<>();
+ Mockito.doAnswer(invocation -> {
+ submitted.add(invocation.getArgument(0));
+ boolean success = submitted.size() != failedBatch;
+ return
OMResponse.newBuilder().setCmdType(OzoneManagerProtocolProtos.Type.PurgeDirectories)
+ .setStatus(success ? OzoneManagerProtocolProtos.Status.OK
+ : OzoneManagerProtocolProtos.Status.INTERNAL_ERROR)
+ .setSuccess(success).build();
+ }).when(subject).submitRequest(Mockito.any(OMRequest.class));
+
+ subject.optimizeDirDeletesAndSubmitRequest(subDirs, purgeList, null,
Time.monotonicNow(), keyManager,
+ kv -> true, kv -> true, Collections.emptyMap(), null, 1, new
AtomicInteger(10));
+
+ assertThat(submitted).hasSize(failedBatch == 0 ? 3 : failedBatch);
+ assertThat(subject.getDeletedDirsCount()).isEqualTo(failedBatch == 0 ? 3
: failedBatch - 1);
+ assertThat(subject.getMovedDirsCount()).isEqualTo(failedBatch == 2 ? 1 :
0);
Review Comment:
nit: Could we list the expected counts in a `@CsvSource`? The `failedBatch`
ternaries are hard to follow, and `0` meaning no failure isn't obvious.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/service/TestDirectoryDeletingService.java:
##########
@@ -258,6 +270,157 @@ void testUpdateAndRestart() throws Exception {
.isEqualTo(newInterval.toMillis());
}
+ @ParameterizedTest
+ @CsvSource({"1, true, false", "2, true, false", "3, true, false",
+ "1, false, false", "2, false, false", "3, false, false",
+ "1, true, true", "2, true, true", "3, true, true",
+ "1, false, true", "2, false, true", "3, false, true"})
Review Comment:
Could we reduce these to four cases? The 12 cases take about two minutes
locally, and `failedBatch=1` adds no coverage since nothing commits. The two
flush points with both failure modes should cover it.
```suggestion
@CsvSource({"2, true, false", "2, false, true", "3, true, true", "3,
false, false"})
```
--
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]
