add check to gc to validate metadata scan is complete

[EdColeman]

Add validation that dir and prevRow are present when the gc is scanning for in-use file references. If a row is processed and the dir and prevRow entries are not present, an IllegalStateException is throw to abort the current gc cycle.

This is to help detect / prevent partial metadata scans from missing file references that are in use.

Fixes #3696 for 1.10

[keith-turner]

Could add a third test for tablet row that has files, but no dir or prev row.

[EdColeman]

Looking at this but my first attempts fail to detect when there are only file entries - I think it is that the row is not being updated - and could be an artifact of the test env.

[EdColeman]

I think the tests added in 01e47c6 cover this?

[EdColeman]

@keith-turner f98bb39 tries to account for having at least one table / tablet scanned. Not sure if this is the correct approach or a valid assumption. For 1.x it may be (rep, and trace?), but not sure if there is a better way?

The idea was that if only ~del markers are seen and no rows, then probably best not to proceed.

And if there are no tablets - well there should not be much to gc anyway...

[EdColeman]

I misinterpreted the suggestion. b187550 adds test for case where only prev row is present, The checking for at least one tablet seen was not necessary and has been reverted with b187550

[keith-turner]

The following is what I was thinking about, a test where there is a file but no dir or prev row.

  @Test
  public void testNoPrevRowNoDir() throws Exception {

    GarbageCollectionAlgorithm gca = new GarbageCollectionAlgorithm();

    TestGCE gce = new TestGCE();
    gce.candidates.add("/1636/default_tablet");
    gce.addFileReference("b", "m", "hdfs://foo.com:6000/user/foo/tables/b/t-0/F00.rf");
    assertThrows(IllegalStateException.class, () -> gca.collect(gce));
  }

[EdColeman]

test added with 3bba72a

[ctubbsii]

Just mentioning that this is a mitigation for #608 (would like that to be noted in the eventual git commit when this gets merged).

[ctubbsii]

My suggestions are comments/style. Functionally, this PR seems good to me. Not sure how difficult it will be to merge forward into 2.1 and main, though. If there are any conflicts in the gc algorithm class, my suggestion to move the tracker class to its own file would definitely help reduce conflicts and make the merge easier, in addition to other benefits of being rigorous about encapsulation and separation of responsibilities.

[ctubbsii]

This tracker class is now a substantial portion of the algorithm class file. I would consider moving this class out to its own separate file in the same package. Since it's a static class and shouldn't be tightly coupled to any of the algorithm class' state, this should be easy to do.

[EdColeman]

The tracker class is not expected to be used outside of the GarbageCollectionAlgorithm - it is not a general class, more of an implementation detail. Moving it outside of the GCA seems to imply that other classes might want to use it. Currently, it was not designed for anything else other than tracking the metadata seen in the context of the GCA metadata scan. If other classes need similar functionality, then this could be refactored to a utility somewhere? But until that need is identified, keeping it internal to GCA seems proper.

[EdColeman]

In 3bba72a the class was renamed MetadataRowReadTracker. Looking at porting the check to 2.1, Ample changes / simplifies the check and this class is not needed to track reading a row-at-at-time. Because this is a one-off and will not be carried forward, I favor keeping this enclosed with the GCA for now.