Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoids holding tablet metadata for queued compactions #5247

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

keith-turner
Copy link
Contributor

@keith-turner keith-turner commented Jan 11, 2025

This change fixes #5188. Unfortunately it touches a lot of code because of cascading dependencies in the code. It would be difficult to break this into a smaller commit. These changes do reduce some of those dependencies though.

There are two major advantages after this change. First tablet metadata is no longer kept in memory for queued compactions. Second the tablet metadata is no longer read during compaction reservation. Before this change the following would happen.

  1. TGW would find a tablet to compact and enqueue compaction jobs+tablet metadata.
  2. Eventually when a compactor requested a job it would yank job+tablet metadata off the queue.
  3. To reserve the compaction a lot of complex analysis was done in the coordinator and then a conditional mutation was submitted. The conditional mutation would require all data involved in the complex analysis to be the same.
  4. If the conditional mutation failed then the coordinator would reread the tablet metadata and go back to step 3.

After this change the following happens in the code.

  1. TGW would find a tablet to compact and enqueue a new class called ResolvedCompactionJob. This new class takes the compaction job and tablet metadata and computes all information needed for the compaction later. The TabletMetadata object is no longer refrenced by this class after the constructor returns. So this class will use much less memory on the queue for the case when tablet have lots of files.
  2. Eventually when a compactor requested a job it would yank a ResolvedCompactionJob off the queue.
  3. All of the complex analysis to determine if a compaction can start is now done in the conditional mutation instead of in the coordinator. To enable this, new functionality was added to Ample including the new TabletMetadataCheck interface, TabletMetadataCheckIterator, and the CompactionReservationCheck class. With these changes its now super easy to write a conditional check that will do arbitrary analysis of TabletMetadata prior to committing a mutation.
  4. Since the analysis is done in the conditional mutation there is no longer a need to retry. If the mutation fails then we know the compaction can not run.

The following were some supporting changes that had to be made.

  • Took methods for encoding KeyExtent as base64 from TabletManagementParameters and moved them to KeyExtent because this was needed in the new TabletMetadataCheckIterator.
  • Move TabletMetadata out of the compaction queues, which made those more independent but was a big change. The main change here is that instead of adding TabletMetadata, List<CompactionJob> to the compaction queue now KeyExtent, List<CompactionJob> is added. This required changing the test for these classes and the code that interacts with them in CompactionCoordinator creating lots of diff.
  • Moved CompactionCoordinatorTest.testCanReserve() to CompactionReservationCheckTest.testCanReserve() and changed the code to work with the new CompactionReservationCheck class. These are test of the complex logic that used to run in the coordinator and now runs in the tablet server as part of a conditional mutation.
  • Removed conditional checks from Ample related to compactions that were no longer used after these changes. There was code for requiring a set of files not not be compacting.

The new TabletMetadataCheck functionality of ample could be used to simplify other conditional mutation checks of tablet metadata. It made the compaction reservation code much simpler and easier to understand. This code could be further improved if #5244 were implemented.

This change fixes apache#5188.  Unfortunately it touches a lot of code because
of cascading dependencies in the code.  It would be difficult to break
this into a smaller commit.  These changes do reduce some of those
dependencies though.

There are two major advantages after this change.  First tablet metadata
is no longer kept in memory for queued compactions.  Second the tablet
metadata is no longer read during compaction reservation.  Before this
change the following would happen.

 1. TGW would find a tablet to compact and enqueue compaction
    jobs+tablet metadata.
 2. Eventually when a compactor requested a job it would yank job+tablet
    metadata off the queue.
 3. To reserve the compaction a lot of complex analysis was done in the
    coordinator and then a conditional mutation was submitted.  The
    conditional mutation would require all data involved in the complex
    analysis to be the same.
 4. If the conditional mutation failed then the coordinator would
    reread the tablet metadata and go back to step 3.

After this change the following happens in the code.

 1. TGW would find a tablet to compact and enqueue a new class called
    ResolvedCompactionJob.  This new class takes the compaction job and
    tablet metadata and computes all information needed for the compaction
    later.  The TabletMetadata object is no longer refrenced by this class
    after the constructor returns.  So this class will use much less memory
    on the queue for the case when tablet have lots of files.
 2. Eventually when a compactor requested a job it would yank a
    ResolvedCompactionJob off the queue.
 3. All of the complex analysis to determine if a compaction can start
    is now done in the conditional mutation instead of in the
    coordinator.  To enable this, new functionality was added to Ample
    including the new TabletMetadataCheck interface,
    TabletMetadataCheckIterator, and the CompactionReservationCheck class.
    With these changes its now super easy to write a conditional check that
    will do arbitrary analysis of TabletMetadata prior to committing a
    mutation.
 4. Since the analysis is done in the conditional mutation there is no
    longer a need to retry.  If the mutation fails then we know the
    compaction can not run.

The following were some supporting changes that had to be made.

 * Took methods for encoding KeyExtent as base64 from
   TabletManagementParameters and moved the KeyExtent because this was
   needed in the new TabletMetadataCheckIterator.
 * Move TabletMetadata out of the compaction queues, which made those
   more independent but was a big change.  The main change here is that
   instead of adding `TabletMetadata, List<CompactionJob>` to the
   compaction queue now `KeyExtent, List<CompactionJob>` is added.  This
   required changing the test for these classes and the code that interacts
   with them in CompactionCoordinator creating lots of diff.
 * Moved CompactionCoordinatorTest.testCanReserve() to
   CompactionReservationCheckTest.testCanReserve() and changed the code
   to work with the new CompactionReservationCheck class.  These are test
   of the complex logic that used to run in the coordinator and now runs in
   the tablet server as part of a conditional mutation.
 * Removed conditional checks from Ample related to compactions that
   were no longer used after these changes.  There was code for
   requiring a set of files not not be compacting.

The new TabletMetadataCheck functionality of ample could be used to
simplify other conditional mutation checks of tablet metadata.  It made
the compaction reservation code much simpler and easier to understand.
This code could be further improved if apache#5244 were implemented.
@keith-turner keith-turner changed the title Avoids holding tablet metadata for compaction reservation Avoids holding tablet metadata for queued compactions Jan 11, 2025
tabletRow, expectedMetaRow);

var colsToRead = check.columnsToRead();
// TODO actually use columns in seek and TabletMetadata construction
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this TODO and open a follow on issue before merging

Copy link
Contributor

@cshannon cshannon Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this TODO and open a follow on issue before merging

I was actually just getting ready to ask you if you planned to do this now or in a future PR

Copy link
Contributor

@cshannon cshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i took a look at this today for quite a while and I didn't see any obvious issues but I'll probably take another look tomorrow when I have some more time as well to see if I missed anything. In general I think the changes are quite nice, the new TabletMetadataCheck is pretty neat and I can see how there could be a lot of use for it in the future use cases.

Something else I was wondering was if we could/should replace any of the existing conditional mutations on Ample with implementations of TabletMetadataCheck instead. For example, instead of having to use the SetEncodingIterator for comparing files it might be simpler to use TabletMetadataCheck instead and just get access to the TabletMetadata there to compare to the set of files vs having to encode and check for equality.

* This interface facilitates atomic checks of tablet metadata prior to updating tablet metadata.
* The way it is intended to be used is the following.
* <ol>
* <li>On the client side a TabletMetadataCheck object is created and passed to Ample</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there is any good way to specifically validate that an implementation can be correctly serialized by gson, especially because we've configured our gson to not allow unsafe usage (so we require a default constructor and not using final etc). I'm guessing the only way is just to to make sure there are tests that use the new check in a conditional mutation and verify it works which is probably fine as we should be writing tests anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think making sure the impl is tested is the best option for that.

expectedExtent = KeyExtent.fromBase64(options.get(EXTENT_KEY));
this.source = source;
} catch (ReflectiveOperationException e) {
throw new IllegalStateException(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be useful to add any logging here for a ClassNotFoundException if the impl isn't on the server classpath? It may not be necessary if this exception will be logged higher up when the IllegalStateException is thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on if the existing exception messages include the class name. Could add the class name to the IllegalStateException to be sure.

tabletRow, expectedMetaRow);

var colsToRead = check.columnsToRead();
// TODO actually use columns in seek and TabletMetadata construction
Copy link
Contributor

@cshannon cshannon Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this TODO and open a follow on issue before merging

I was actually just getting ready to ask you if you planned to do this now or in a future PR

tm -> tm.getExternalCompactions().containsKey(externalCompactionId),
() -> "compaction reservation");
try {
try (var tabletsMutator = ctx.getAmple().conditionallyMutateTablets()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the changes here are really nice, it's definitely a lot simpler. I think it would make moving the reservation to the Compactor easier as well if we implement #4978

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid having tablet metadata in memory for queued compaction jobs
2 participants