Remove continue point from Garbage Collector #2214

jmark99 · 2021-07-26T16:35:48Z

Updated Garbage Collection code to no longer use a continue point when processing deletion candidates. The GC now uses an iterator that lasts during the lifetime of a GC cycle.

The GarbageCollectionTest was updated to work with the update, as was the GC integration test.

Closes #1351

Updated Garbage Collection code to no longer use a continue point when processing deletion candidates. The GC now uses an iterator that lasts during the lifetime of a GC cycle. The GarbageCollectionTest was updated to work with the update, as was the GC integration test. Closes apache#1351

Correcting some formatting issues.

milleruntime

I think these changes look good but I had to spend a good chunk of time learning how the GC currently uses the continue point. I would like to look at the changes closer and do some testing but I would be OK if you merged before I get the chance.

jmark99 · 2021-07-26T19:58:26Z

Thanks for taking a look @milleruntime. Looking more closely is a good idea since we definitely want the GC to work as expected. I've run the unit and IT tests as well as the sunny profile. Additionally, I ran the GC test in the accumulo-testing repo, although I'd like to run it for a longer period of time. I may wait and see if @keith-turner would like to take a look before merging since he had created the initial ticket.

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

milleruntime · 2021-07-27T13:24:42Z

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

   */
-  boolean getCandidates(String continuePoint, List<String> candidates)
-      throws TableNotFoundException;
+  void processCandidates() throws TableNotFoundException, IOException;


I wonder if this method should be split up. Previously we had one method to gather the candidates and then another that processed them.

Given that the processCandidates method is only about three lines, not counting the loop, I'm not sure it is worth it. I could re-create a method, getCandidates, that would be a one-liner returning the required Iterator and then gather the candidates and process them separately.

In the current code using a continue point, the getCandidates method does get potential candidates and then returns to the collect method to do the processing. With the updated code using the iterator there is less of a separation between those two actions. Passing the iterator around between classes could become confusing as well. I had tried to do something similar to what you are suggesting but I was having issues with the iterator not keeping track of the candidates properly after being passed between multiple methods and classes. Most likely coder error, but I eventually caved and went back to the current form that you see now. I could try again if you feel strongly about it.

My concern is creating more complexity in code that is already overly complex. If we are going to change the interface, lets design it in a way that reduces complexity. It appears that the GC interfaces were done so users could potentially write their own but I don't think that ever happened. This then makes me think that if we want to make the GC pluggable, lets do it right (either as a part of the SPI or API). But this seems like much too daunting of a task and I don't think there is much for users to gain by writing their own. That brings me to the idea that we should at least refactor the GC to make it more readable and maintainable.

Unfortunately, I don't have a suggestion on how that could be done for just these few methods. I would have to spend a lot more time looking at the GC.

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

milleruntime · 2021-07-27T13:34:33Z

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

-      confirmDeletesTrace(gce, candidateMap);
-      gce.incrementInUseStat(origSize - candidateMap.size());
+    deleteConfirmed(gce, candidateMap);
+  }


The previous collect() method was confusing but it was the only public method of the GarbageCollectionAlgorithm so it was at least easier to follow from the one entry point. Since you are refactoring the methods, it might be a good opportunity to clean it up and make it easier to follow. The GC was already confusing but the way you split up the methods, is more confusing to me. It looks like you have the correct business logic between the methods but I think it could be organized better.

It seems the simplest solution would be to keep the same methods but just drop the continue point. Was there a reason you had to create new methods? I saw your comment in the tests about ConcurrentModificationException but I am not seeing where that could happen.

I'll take another look, but primarily I was following @keith-turner format suggestion in the inital ticket. Since the call to Ample takes place in the classes implementing the GarbageCollectionEnvironment code, the few short lines seemed to belong in a single method. Not sure if better naming of methods would clear things up.

Realistically the call to collect could be dropped altogether and just be replaced by a call to processCandidates (since that is all it calls anyway). I just kept it so the existing GC initial call would not be changed. Then the GCA could have a public method called' processCandidates' (or maybe even processDeletionCandidates). That seems clearer to me than using 'collect'. Then the the collectBatch call does what is implies. It collects a batch of potential deletion candidates and processes them. Maybe even get away from the 'collect' naming altogether and call it 'processBatch' or something similar.

I suspect the ConcurrentModificationException occurs in the test. I would suggest refactoring the test GCE so that this does not happen rather than the GCA. The GarbageCollectionAlgorithm was designed to pull the logic of collection into a single place where it could be unit tested. This change pushes some logic of collection into the GarbCollEnv, resulting in a need to change logic in the test which means the full algorithm is not being tested.

I suspect this is what was causing the CME in the unit test

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

Line 81 in 146b1d3

this.candidates.removeAll(candidateMap.values());

@keith-turner that was exactly the cause.

Still have not looked in depth, so this may not be workable. Thinking one possible way to avoid the CME would be to make the getCandidates() method in TestGCE return a snapshot. Something like :

Iterator<String> getCandidates() { return List.copyOf(candidates).iterator(); }

…onTest.java Remove an extra character in the comment. Co-authored-by: Mike Miller <mmiller@apache.org>

…onAlgorithm.java Typo correction. Co-authored-by: Mike Miller <mmiller@apache.org>

keith-turner · 2021-07-27T14:38:09Z

@jmark99 I just took an initial look at this. I plan to take a more in-depth look later this afternoon.

EdColeman · 2021-07-27T15:15:36Z

@mjwall might have additional insight if this overlaps with the issue that he has been tracking down.

Formatting update.

Refactor after updating getCandidates to return an iterator. Flow is now more closely linked to original code but without use of continue point.

jmark99 · 2021-07-27T20:01:34Z

@milleruntime , @keith-turner I did some re-factoring based upon @keith-turner suggestion concerning the getCandidates method. It now returns an iterator. With that change, the flow now more closely resembles the original flow of the code but without the continue point.

keith-turner

@jmark99 the latest changes look great. Made one small comment about making a method private, if that is possible.

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

test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java

Changed collectBatch access from public to private.

milleruntime

Changes look better, thanks. Only other thing I had was the name of one method.

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

mjwall · 2021-07-28T12:10:34Z

I can look at this tonight if it is still open

Renamed collectBatch method to deleteBatch.

jmark99 · 2021-07-28T15:59:08Z

@mjwall I will wait until tomorrow to merge if you wish to take a look tonight.

mjwall · 2021-07-29T12:01:27Z

This looks good to me @jmark99 . It is unrelated to what I am tracking in #1916. First I have looked at Ample, cool abstraction. Hope it is enough <pun intended>

ctubbsii

I'm a bit late to this PR, but overall it looks great. I had only one comment about how we're tracing the candidate retrieval. Also I saw ConcurrentDeleteTableIT time out after this was committed, but I haven't had a chance to run it again to see if it's a fluke. Occasional timeouts are normal, so it might not be an issue at all.

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

jmark99 requested a review from keith-turner July 26, 2021 16:35

jmark99 added 2 commits July 26, 2021 14:18

Remove continue point from Garbage Collector

40a4998

Correcting some formatting issues.

milleruntime reviewed Jul 26, 2021

View reviewed changes

milleruntime reviewed Jul 27, 2021

View reviewed changes

jmark99 and others added 2 commits July 27, 2021 09:47

Update server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollecti…

cfb6a07

…onTest.java Remove an extra character in the comment. Co-authored-by: Mike Miller <mmiller@apache.org>

Update server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollecti…

366629f

…onAlgorithm.java Typo correction. Co-authored-by: Mike Miller <mmiller@apache.org>

EdColeman requested a review from mjwall July 27, 2021 15:13

jmark99 added 2 commits July 27, 2021 11:59

Remove continue point from Garbage Collector

ab79bba

Formatting update.

Remove continue point from Garbage Collector

54c5488

Refactor after updating getCandidates to return an iterator. Flow is now more closely linked to original code but without use of continue point.

keith-turner approved these changes Jul 27, 2021

View reviewed changes

server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java Outdated Show resolved Hide resolved

test/src/main/java/org/apache/accumulo/test/functional/GarbageCollectorIT.java Show resolved Hide resolved

Remove continue point from Garbage Collector

753ff42

Changed collectBatch access from public to private.

milleruntime approved these changes Jul 28, 2021

View reviewed changes

server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java Outdated Show resolved Hide resolved

Remove continue point from Garbage Collector

e737e29

Renamed collectBatch method to deleteBatch.

jmark99 merged commit 485114a into apache:main Jul 29, 2021

ctubbsii reviewed Jul 29, 2021

View reviewed changes

server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java Show resolved Hide resolved

server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java Show resolved Hide resolved

jmark99 deleted the gh1351 branch October 25, 2021 17:17

ctubbsii added this to the 2.1.0 milestone Jul 12, 2024

Remove continue point from Garbage Collector #2214

Remove continue point from Garbage Collector #2214

Uh oh!

Conversation

jmark99 commented Jul 26, 2021

Uh oh!

milleruntime left a comment

Choose a reason for hiding this comment

Uh oh!

jmark99 commented Jul 26, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner commented Jul 27, 2021

Uh oh!

EdColeman commented Jul 27, 2021

Uh oh!

jmark99 commented Jul 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

milleruntime left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mjwall commented Jul 28, 2021

Uh oh!

jmark99 commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjwall commented Jul 29, 2021

Uh oh!

ctubbsii left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jmark99 commented Jul 27, 2021 •

edited

Loading

jmark99 commented Jul 28, 2021 •

edited

Loading