SourabhBadhya commented on code in PR #4740: URL: https://github.com/apache/hive/pull/4740#discussion_r1463084357
########## ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/handler/TestAbortedTxnCleaner.java: ########## @@ -282,9 +284,19 @@ public void testCleaningOfAbortedDirectoriesBelowBase() throws Exception { cleaner.setCleanupHandlers(Arrays.asList(mockedTaskHandler)); cleaner.run(); - Mockito.verify(mockedFSRemover, Mockito.times(1)).clean(any(CleanupRequest.class)); + Mockito.verifyNoInteractions(mockedFSRemover); Review Comment: Why this change in test? If the cleaner has run then ideally to delete the files there is call made to FsRemover mocked object. Also there is only one compaction request on this table, so this request cannot be skipped. ########## ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/handler/TestAbortedTxnCleaner.java: ########## @@ -282,9 +284,19 @@ public void testCleaningOfAbortedDirectoriesBelowBase() throws Exception { cleaner.setCleanupHandlers(Arrays.asList(mockedTaskHandler)); cleaner.run(); - Mockito.verify(mockedFSRemover, Mockito.times(1)).clean(any(CleanupRequest.class)); + Mockito.verifyNoInteractions(mockedFSRemover); Mockito.verify(mockedTaskHandler, Mockito.times(1)).getTasks(); + directories = getDirectories(conf, t, null); + // Both base and delta files are present since the cleaner skips them as there is a newer write. + Assert.assertEquals(5, directories.size()); + Assert.assertEquals(1, directories.stream().filter(dir -> dir.getName().startsWith(AcidUtils.BASE_PREFIX)).count()); + + // Run compaction and clean up + startInitiator(); + startWorker(); + startCleaner(); + Review Comment: I am not sure why we are calling worker and cleaner again. The scope of this test is to remove committed and aborted deltas below the base (compacted) file in a cleaner the first time. ########## ql/src/test/org/apache/hadoop/hive/ql/txn/compactor/handler/TestAbortedTxnCleaner.java: ########## @@ -315,7 +327,6 @@ public void testAbortedCleaningWithThreeTxnsWithDiffWriteIds() throws Exception addDeltaFile(t, null, writeId1, writeId1, 2); addDeltaFile(t, null, writeId2, writeId2, 2); - ms.abortTxns(Collections.singletonList(openTxnId2)); Review Comment: Why are we not aborting txns? This is what is tested here. -- 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: gitbox-unsubscr...@hive.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For additional commands, e-mail: gitbox-h...@hive.apache.org