Skip to content

Commit 1ccc3ef

Browse files
ddanielrddanielr
authored
Remove Experimental property feature flag (#6416)
* Remove Experimental property feature flag Removes the `gc.remove.in.use.candidates` feature flag property that was restricting the gc behavior from removing gc candidates for files that were still referenced by other tablets. * Replace hard-coded paths with method calls Replaced the hardcoded path values with method calls for tests that used the exact same path for file reference creation.
1 parent 7c7463b commit 1ccc3ef

5 files changed

Lines changed: 71 additions & 100 deletions

File tree

‎core/src/main/java/org/apache/accumulo/core/conf/Property.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -996,11 +996,6 @@ public enum Property {
996996
"The number of threads used to delete write-ahead logs and recovery files.", "2.1.4"),
997997
GC_DELETE_THREADS("gc.threads.delete", "16", PropertyType.COUNT,
998998
"The number of threads used to delete RFiles.", "1.3.5"),
999-
@Experimental
1000-
GC_REMOVE_IN_USE_CANDIDATES("gc.remove.in.use.candidates", "false", PropertyType.BOOLEAN,
1001-
"GC will remove deletion candidates that are in-use from the metadata location. "
1002-
+ "This is expected to increase the speed of subsequent GC runs.",
1003-
"2.1.3"),
1004999
@Deprecated(since = "2.1.1", forRemoval = true)
10051000
GC_TRASH_IGNORE("gc.trash.ignore", "false", PropertyType.BOOLEAN,
10061001
"Do not use the Trash, even if it is configured.", "1.5.0"),

‎server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -529,16 +529,6 @@ boolean inSafeMode() {
529529
return context.getConfiguration().getBoolean(Property.GC_SAFEMODE);
530530
}
531531

532-
/**
533-
* Checks if InUse Candidates can be removed.
534-
*
535-
* @return value of {@link Property#GC_REMOVE_IN_USE_CANDIDATES}
536-
*/
537-
@Override
538-
public boolean canRemoveInUseCandidates() {
539-
return context.getConfiguration().getBoolean(Property.GC_REMOVE_IN_USE_CANDIDATES);
540-
}
541-
542532
/**
543533
* Moves a file to trash. If this garbage collector is not using trash, this method returns false
544534
* and leaves the file alone. If the file is missing, this method returns false as opposed to

‎server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -198,9 +198,7 @@ private void removeCandidatesInUse(GarbageCollectionEnvironment gce,
198198
Set<TableId> tableIdsAfter = gce.getCandidateTableIDs();
199199
ensureAllTablesChecked(Collections.unmodifiableSet(tableIdsBefore),
200200
Collections.unmodifiableSet(tableIdsSeen), Collections.unmodifiableSet(tableIdsAfter));
201-
if (gce.canRemoveInUseCandidates()) {
202-
gce.deleteGcCandidates(candidateEntriesToBeDeleted, GcCandidateType.INUSE);
203-
}
201+
gce.deleteGcCandidates(candidateEntriesToBeDeleted, GcCandidateType.INUSE);
204202
}
205203

206204
private long removeBlipCandidates(GarbageCollectionEnvironment gce,

‎server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionEnvironment.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@
2929
import java.util.stream.Stream;
3030

3131
import org.apache.accumulo.core.client.TableNotFoundException;
32-
import org.apache.accumulo.core.conf.Property;
3332
import org.apache.accumulo.core.data.TableId;
3433
import org.apache.accumulo.core.gc.GcCandidate;
3534
import org.apache.accumulo.core.gc.Reference;
@@ -51,13 +50,6 @@ public interface GarbageCollectionEnvironment {
5150
*/
5251
Iterator<GcCandidate> getCandidates() throws TableNotFoundException;
5352

54-
/**
55-
* Used for determining if deletion of InUse candidates is enabled.
56-
*
57-
* @return value of {@link Property#GC_REMOVE_IN_USE_CANDIDATES}
58-
*/
59-
boolean canRemoveInUseCandidates();
60-
6153
/**
6254
* Given an iterator to a deletion candidate list, return a sub-list of candidates which fit
6355
* within provided memory constraints.

‎server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectionTest.java‎

Lines changed: 70 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,6 @@ static class TestGCE implements GarbageCollectionEnvironment {
6262
ArrayList<GcCandidate> fileDeletions = new ArrayList<>();
6363
ArrayList<TableId> tablesDirsToDelete = new ArrayList<>();
6464
TreeMap<String,Status> filesToReplicate = new TreeMap<>();
65-
boolean deleteInUseRefs = false;
6665

6766
private long timestamp = 0L;
6867

@@ -92,11 +91,6 @@ public Iterator<GcCandidate> getCandidates() throws TableNotFoundException {
9291
return List.copyOf(candidates).iterator();
9392
}
9493

95-
@Override
96-
public boolean canRemoveInUseCandidates() {
97-
return deleteInUseRefs;
98-
}
99-
10094
@Override
10195
public List<GcCandidate> readCandidatesThatFitInMemory(Iterator<GcCandidate> candidatesIter) {
10296
List<GcCandidate> candidatesBatch = new ArrayList<>();
@@ -239,7 +233,7 @@ private void assertFileDeleted(TestGCE gce, GcCandidate... candidates) {
239233

240234
private void assertNoCandidatesRemoved(TestGCE gce) {
241235
assertEquals(0, gce.deletedCandidates.size(),
242-
"Deleted Candidates not empty: " + gce.deleteInUseRefs);
236+
"Deleted Candidates not empty: " + gce.deletedCandidates.entrySet());
243237
}
244238

245239
private void assertCandidateRemoved(TestGCE gce, GcCandidateType gcCandidateType,
@@ -248,7 +242,7 @@ private void assertCandidateRemoved(TestGCE gce, GcCandidateType gcCandidateType
248242
assertEquals(gcCandidateType, gce.deletedCandidates.remove(gcCandidate));
249243
}
250244
assertEquals(0, gce.deletedCandidates.size(),
251-
"Deleted Candidates not empty: " + gce.deleteInUseRefs);
245+
"Deleted Candidates not empty: " + gce.deletedCandidates.entrySet());
252246
}
253247

254248
// This test was created to help track down a ConcurrentModificationException error that was
@@ -258,19 +252,20 @@ private void assertCandidateRemoved(TestGCE gce, GcCandidateType gcCandidateType
258252
public void minimalDelete() throws Exception {
259253
TestGCE gce = new TestGCE();
260254

261-
gce.addCandidate("hdfs://foo:6000/accumulo/tables/4/t0/F000.rf");
262-
gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
263-
var candidate = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
264-
gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/6/t0/F006.rf");
255+
var candOne = gce.addCandidate("hdfs://foo:6000/accumulo/tables/4/t0/F000.rf");
256+
var candTwo = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
257+
var candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
258+
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/6/t0/F006.rf");
265259

266-
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
267-
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
268-
gce.addFileReference("6", null, "hdfs://foo.com:6000/accumulo/tables/6/t0/F006.rf");
260+
gce.addFileReference("4", null, candOne.getPath());
261+
gce.addFileReference("4", null, candTwo.getPath());
262+
gce.addFileReference("6", null, candFour.getPath());
269263

270264
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
271265
gca.collect(gce);
272266

273-
assertFileDeleted(gce, candidate);
267+
assertFileDeleted(gce, candThree);
268+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne, candTwo, candFour);
274269
}
275270

276271
@Test
@@ -279,40 +274,40 @@ public void testBasic() throws Exception {
279274

280275
var candOne = gce.addCandidate("hdfs://foo:6000/accumulo/tables/4/t0/F000.rf");
281276
var candTwo = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
282-
gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
277+
var candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
283278

284-
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
285-
gce.addFileReference("4", null, "hdfs://foo:6000/accumulo/tables/4/t0/F001.rf");
279+
gce.addFileReference("4", null, candOne.getPath());
280+
gce.addFileReference("4", null, candTwo.getPath());
286281
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0//F002.rf");
287-
gce.addFileReference("5", null, "hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
282+
gce.addFileReference("5", null, candThree.getPath());
288283

289284
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
290285

291-
gca.collect(gce);
292-
assertFileDeleted(gce);
293-
294-
// Remove the reference to this flush file, run the GC which should not trim it from the
295-
// candidates, and assert that it's gone
296-
gce.removeFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
286+
// Remove the reference to a file in the candidates should cause it to be removed
287+
gce.removeFileReference("4", null, candOne.getPath());
297288
gca.collect(gce);
298289
assertFileDeleted(gce, candOne);
290+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candTwo, candThree);
299291

300-
// Removing a reference to a file that wasn't in the candidates should do nothing
301-
gce.removeFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F002.rf");
302-
gca.collect(gce);
303-
assertFileDeleted(gce);
304-
292+
candTwo = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
305293
// Remove the reference to a file in the candidates should cause it to be removed
306-
gce.removeFileReference("4", null, "hdfs://foo:6000/accumulo/tables/4/t0/F001.rf");
294+
gce.removeFileReference("4", null, candTwo.getPath());
307295
gca.collect(gce);
308296
assertFileDeleted(gce, candTwo);
297+
assertNoCandidatesRemoved(gce);
309298

310299
// Adding more candidates which do not have references should be removed
311-
var candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F003.rf");
312-
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F004.rf");
300+
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F003.rf");
301+
var candFive = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F004.rf");
313302
gca.collect(gce);
314-
assertFileDeleted(gce, candThree, candFour);
303+
assertFileDeleted(gce, candFour, candFive);
304+
assertNoCandidatesRemoved(gce);
315305

306+
// Removing a reference to a file that wasn't in the candidates should do nothing
307+
gce.removeFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F002.rf");
308+
gca.collect(gce);
309+
assertFileDeleted(gce);
310+
assertNoCandidatesRemoved(gce);
316311
}
317312

318313
/*
@@ -328,7 +323,7 @@ public void testBasic2() throws Exception {
328323

329324
var candOne = gce.addCandidate("hdfs://foo:6000/accumulo/tables/4/t0/F000.rf");
330325
var candTwo = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
331-
gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
326+
var candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
332327

333328
int counter = 0;
334329
// items to be removed from candidates
@@ -358,37 +353,44 @@ public void testBasic2() throws Exception {
358353
counter++;
359354
}
360355

361-
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
362-
gce.addFileReference("4", null, "hdfs://foo:6000/accumulo/tables/4/t0/F001.rf");
356+
gce.addFileReference("4", null, candOne.getPath());
357+
gce.addFileReference("4", null, candTwo.getPath());
363358
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0//F002.rf");
364-
gce.addFileReference("5", null, "hdfs://foo.com:6000/accumulo/tables/5/t0/F005.rf");
359+
gce.addFileReference("5", null, candThree.getPath());
365360

366361
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
367362

368363
gca.collect(gce);
369364
assertFileDeleted(gce, toBeRemoved);
365+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne, candTwo, candThree);
370366

371-
// Remove the reference to this flush file, run the GC which should not trim it from the
372-
// candidates, and assert that it's gone
373-
gce.removeFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
367+
// Add the candidate back in for the file removal
368+
candOne = gce.addCandidate("hdfs://foo:6000/accumulo/tables/4/t0/F000.rf");
369+
// Remove the reference to a file in the candidates should cause it to be removed
370+
gce.removeFileReference("4", null, candOne.getPath());
374371
gca.collect(gce);
375372
assertFileDeleted(gce, candOne);
373+
assertNoCandidatesRemoved(gce);
376374

377375
// Removing a reference to a file that wasn't in the candidates should do nothing
378376
gce.removeFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F002.rf");
379377
gca.collect(gce);
380378
assertFileDeleted(gce);
379+
assertNoCandidatesRemoved(gce);
381380

382381
// Remove the reference to a file in the candidates should cause it to be removed
383-
gce.removeFileReference("4", null, "hdfs://foo:6000/accumulo/tables/4/t0/F001.rf");
382+
candTwo = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
383+
gce.removeFileReference("4", null, candTwo.getPath());
384384
gca.collect(gce);
385385
assertFileDeleted(gce, candTwo);
386+
assertNoCandidatesRemoved(gce);
386387

387388
// Adding more candidates which do no have references should be removed
388-
var candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F003.rf");
389-
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F004.rf");
389+
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F003.rf");
390+
var candFive = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F004.rf");
390391
gca.collect(gce);
391-
assertFileDeleted(gce, candThree, candFour);
392+
assertFileDeleted(gce, candFour, candFive);
393+
assertNoCandidatesRemoved(gce);
392394
}
393395

394396
/**
@@ -398,19 +400,20 @@ public void testBasic2() throws Exception {
398400
public void emptyPathsTest() throws Exception {
399401
TestGCE gce = new TestGCE();
400402

401-
gce.addCandidate("hdfs://foo:6000/accumulo/tables/4//t0//F000.rf");
402-
gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4//t0//F001.rf");
403-
var candidate = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5//t0//F005.rf");
404-
gce.addCandidate("hdfs://foo.com:6000/accumulo//tables//6/t0/F006.rf");
403+
var candOne = gce.addCandidate("hdfs://foo:6000/accumulo/tables/4//t0//F000.rf");
404+
var candTwo = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4//t0//F001.rf");
405+
var candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/5//t0//F005.rf");
406+
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo//tables//6/t0/F006.rf");
405407

406-
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4//t0//F000.rf");
407-
gce.addFileReference("4", null, "hdfs://foo.com:6000/accumulo/tables/4//t0//F001.rf");
408-
gce.addFileReference("6", null, "hdfs://foo.com:6000/accumulo//tables//6/t0/F006.rf");
408+
gce.addFileReference("4", null, candOne.getPath());
409+
gce.addFileReference("4", null, candTwo.getPath());
410+
gce.addFileReference("6", null, candFour.getPath());
409411

410412
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
411413
gca.collect(gce);
412414

413-
assertFileDeleted(gce, candidate);
415+
assertFileDeleted(gce, candThree);
416+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne, candTwo, candFour);
414417
}
415418

416419
@Test
@@ -432,7 +435,10 @@ public void testRelative() throws Exception {
432435
// All candidates currently have references
433436
gca.collect(gce);
434437
assertFileDeleted(gce);
438+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne, candTwo, candThree);
435439

440+
// Add a single reference but three possible locations for delete.
441+
candOne = gce.addCandidate("/4/t0/F000.rf");
436442
List<String[]> refsToRemove = new ArrayList<>();
437443
refsToRemove.add(new String[] {"4", "/t0/F000.rf"});
438444
refsToRemove.add(new String[] {"5", "../4/t0/F000.rf"});
@@ -444,27 +450,37 @@ public void testRelative() throws Exception {
444450
gce.removeFileReference(refsToRemove.get(i)[0], null, refsToRemove.get(i)[1]);
445451
gca.collect(gce);
446452
assertFileDeleted(gce);
453+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candOne);
454+
candOne = gce.addCandidate("/4/t0/F000.rf");
447455
}
448456

449457
gce.removeFileReference(refsToRemove.get(2)[0], null, refsToRemove.get(2)[1]);
450458
gca.collect(gce);
451459
assertFileDeleted(gce, candOne);
460+
assertNoCandidatesRemoved(gce);
452461

462+
candThree = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F001.rf");
453463
gce.removeFileReference("4", null, "/t0/F001.rf");
454464
gca.collect(gce);
455465
assertFileDeleted(gce, candThree);
466+
assertNoCandidatesRemoved(gce);
456467

457468
// add absolute candidate for file that already has a relative candidate
458469
var candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F002.rf");
459470
gca.collect(gce);
460471
assertFileDeleted(gce);
472+
assertCandidateRemoved(gce, GcCandidateType.INUSE, candFour);
461473

474+
candTwo = gce.addCandidate("/4/t0/F002.rf");
475+
candFour = gce.addCandidate("hdfs://foo.com:6000/accumulo/tables/4/t0/F002.rf");
462476
gce.removeFileReference("4", null, "/t0/F002.rf");
463477
gca.collect(gce);
464478
assertFileDeleted(gce, candFour);
479+
assertNoCandidatesRemoved(gce);
465480

466481
gca.collect(gce);
467482
assertFileDeleted(gce, candTwo);
483+
assertNoCandidatesRemoved(gce);
468484
}
469485

470486
@Test
@@ -925,16 +941,9 @@ public void testDeletingInUseReferenceCandidates() throws Exception {
925941
gce.addFileReference("6", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
926942

927943
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
928-
gce.deleteInUseRefs = false;
929944
// All candidates currently have references
930945
gca.collect(gce);
931946
assertFileDeleted(gce);
932-
assertNoCandidatesRemoved(gce);
933-
934-
// Enable InUseRefs to be removed if the file ref is found.
935-
gce.deleteInUseRefs = true;
936-
gca.collect(gce);
937-
assertFileDeleted(gce);
938947
assertCandidateRemoved(gce, GcCandidateType.INUSE, candidate);
939948

940949
var cand1 = gce.addCandidate("/9/t0/F003.rf");
@@ -968,15 +977,6 @@ public void testDeletingRootInUseReferenceCandidates() throws Exception {
968977
gce.addFileReference("+r", null, "hdfs://foo.com:6000/accumulo/tables/4/t0/F000.rf");
969978

970979
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
971-
gce.deleteInUseRefs = false;
972-
// No InUse Candidates should be removed.
973-
gca.collect(gce);
974-
assertFileDeleted(gce);
975-
assertNoCandidatesRemoved(gce);
976-
977-
gce.deleteInUseRefs = true;
978-
// Due to the gce Datalevel of ROOT, InUse candidate deletion is not supported regardless of
979-
// property setting.
980980
gca.collect(gce);
981981
assertFileDeleted(gce);
982982
assertNoCandidatesRemoved(gce);
@@ -1019,9 +1019,6 @@ public void testInUseDirReferenceCandidates() throws Exception {
10191019

10201020
assertEquals(0, gce.candidates.size());
10211021

1022-
// Now enable InUse deletions
1023-
gce.deleteInUseRefs = true;
1024-
10251022
// Add deletion candidate for a directory.
10261023
var candidate = new GcCandidate("6/t-0/", 10L);
10271024
gce.candidates.add(candidate);
@@ -1050,7 +1047,6 @@ public void testInUseScanReferenceCandidates() throws Exception {
10501047
gce.addFileReference("4", null, "/t0/F000.rf");
10511048

10521049
GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();
1053-
gce.deleteInUseRefs = true;
10541050

10551051
gca.collect(gce);
10561052
assertFileDeleted(gce, candTwo);

0 commit comments

Comments
 (0)