[GitHub] [accumulo] keith-turner commented on a diff in pull request #3738: Remove GC candidates found to still be referenced

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #3738:
URL: https://github.com/apache/accumulo/pull/3738#discussion_r1332365405


##########
server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java:
##########
@@ -863,6 +915,45 @@ public void testMissingTableIds() throws Exception {
         && msg.contains("Saw table IDs in ZK that were not in metadata table:"), msg);
   }
 
+  @Test
+  public void testDeletingInUseReferenceCandidates() throws Exception {

Review Comment:
   Those test changes look good.  Played around locally with tracking the candidate type to see if that was workable and it seemed it is.  The following changes are where I tried that.  I only asserted it in one random place, not sure how many more places the type could be asserted.
   
   
   ```diff
   diff --git a/server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java b/server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java
   index 1515910287..6e866610e3 100644
   --- a/server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java
   +++ b/server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java
   @@ -19,6 +19,7 @@
    package org.apache.accumulo.gc;
    
    import static org.junit.jupiter.api.Assertions.assertEquals;
   +import static org.junit.jupiter.api.Assertions.assertNotNull;
    import static org.junit.jupiter.api.Assertions.assertThrows;
    import static org.junit.jupiter.api.Assertions.assertTrue;
    
   @@ -55,7 +56,7 @@ public class GarbageCollectionTest {
    
      static class TestGCE implements GarbageCollectionEnvironment {
        TreeSet<GcCandidate> candidates = new TreeSet<>();
   -    TreeSet<GcCandidate> deletedCandidates = new TreeSet<>();
   +    TreeMap<GcCandidate, GcCandidateType> deletedCandidates = new TreeMap<>();
        ArrayList<String> blips = new ArrayList<>();
        Map<String,Reference> references = new TreeMap<>();
        HashSet<TableId> tableIds = new HashSet<>();
   @@ -123,7 +124,7 @@ public class GarbageCollectionTest {
          if (type.equals(GcCandidateType.INUSE) && this.level.equals(Ample.DataLevel.ROOT)) {
            return;
          }
   -      deletedCandidates.addAll(refCandidates);
   +      refCandidates.forEach(gcCandidate -> deletedCandidates.put(gcCandidate, type));
          this.candidates.removeAll(refCandidates);
        }
    
   @@ -226,12 +227,20 @@ public class GarbageCollectionTest {
    
      private void assertCandidateRemoved(TestGCE gce, GcCandidate... gcCandidates) {
        for (GcCandidate gcCandidate : gcCandidates) {
   -      assertTrue(gce.deletedCandidates.remove(gcCandidate));
   +        assertNotNull(gce.deletedCandidates.remove(gcCandidate));
        }
        assertEquals(0, gce.deletedCandidates.size(),
            "Deleted Candidates not empty: " + gce.deleteInUseRefs);
      }
    
   +  private void assertCandidateRemoved(TestGCE gce, GcCandidateType gcCandidateType, GcCandidate... gcCandidates) {
   +    for (GcCandidate gcCandidate : gcCandidates) {
   +      assertEquals(gcCandidateType, gce.deletedCandidates.remove(gcCandidate));
   +    }
   +    assertEquals(0, gce.deletedCandidates.size(),
   +            "Deleted Candidates not empty: " + gce.deleteInUseRefs);
   +  }
   +
      // This test was created to help track down a ConcurrentModificationException error that was
      // occurring with the unit tests once the GC was refactored to use a single iterator for the
      // collect process. This was a minimal test case that would cause the exception to occur.
   @@ -1012,7 +1021,7 @@ public class GarbageCollectionTest {
        var removedCandidate = gce.addCandidate("6/t-0/F003.rf");
    
        gca.collect(gce);
   -    assertCandidateRemoved(gce, removedCandidate);
   +    assertCandidateRemoved(gce, GcCandidateType.INUSE, removedCandidate);
        assertRemoved(gce);
        // Check and make sure the InUse directory candidates are not removed.
        assertEquals(1, gce.candidates.size());
   
   ```



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org