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

Support dedicating threads to specific types of Fate transactions #5130

Open
dlmarion opened this issue Dec 3, 2024 · 11 comments
Open

Support dedicating threads to specific types of Fate transactions #5130

dlmarion opened this issue Dec 3, 2024 · 11 comments
Assignees
Milestone

Comments

@dlmarion
Copy link
Contributor

dlmarion commented Dec 3, 2024

In main the Manager instantiates two instances of Fate, one for the MetaFateStore and one for the UserFateStore. Each instance of Fate contains a single thread pool of Property.MANAGER_FATE_THREADPOOL_SIZE size which is used to process Fate transactions. The threads in the Fate thread pool try to process transactions in a fair manner, by spending time on a different transaction in the queue when done with the current transactions' step (see #3852 and FateInterleavingIT for more information). The default size of MANAGER_FATE_THREADPOOL_SIZE is 4, which may need to be increased in main, could allow for some transactions to go a long time without making progress (It's possible that this could be the cause of the long delete table transaction times we are seeing in integration tests).

It may be advisable to dedicate some number of threads to specific transaction types. For example, maybe 1 thread dedicated to creating and deleting tables, and a larger thread pool dedicated to bulk imports and compactions. I'm initially thinking of a change to the MANAGER_FATE_THREADPOOL_SIZE property such that instead of it being a count of the number of threads, the property is renamed to MANAGER_FATE_THREADPOOL_CONFIGURATION and contains a json map where the key is a comma separated list of Fate transaction names and the value is the number of threads, like:

{
  "SET_AVAILABILITY,CREATE_TABLE,DELETE_TABLE,CREATE_NAMESPACE,DELETE_NAMESPACE,RENAME_NAMESPACE": 1,
  "BULK_IMPORT": 2,
  "COMPACT": 4,
  "MERGE,SPLIT,IMPORT_TABLE,EXPORT_TABLE": 2
}

The Manager would parse this configuration and start one Fate instance for each Key,Value pair in the map. The Fate instance would be responsible for managing only the types that it's given. Currently MANAGER_FATE_THREADPOOL_SIZE is watched and the size of the threadpool changes when the property changes. That may be harder to do with this idea. I think the name of the transaction is currently serialized into the transaction and I think the transactions can be filtered by type, so this would build upon that capability.

@dlmarion dlmarion added this to the 4.0.0 milestone Dec 3, 2024
@dlmarion
Copy link
Contributor Author

dlmarion commented Dec 3, 2024

It might be useful for COMPACTION_COMMIT to be it's own type with it's own dedicated resources.

@cshannon
Copy link
Contributor

cshannon commented Dec 4, 2024

One other idea I thought of is if we could configure the fate thread pool be to use a PriorityBlockingQueue and then write a comparator to bump the priority of certain fate operation types higher than others so they run first.

This option may not work though because it is not going to be as good as having separate dedicated thread pools to run fate operations by type because if the thread pool is saturated and tied up with long running fate jobs then it still blocks jobs from running, but it at least would bump certain things to the front to run as soon as there are free threads.

@cshannon
Copy link
Contributor

cshannon commented Dec 5, 2024

@kevinrr888 - Is this something you wanted to take a look at? I figure we were involved with the recent FATE changes and updates so one of us could probably work on this and feel free to grab it if you want to but if you were not interested or working on something else I could take a look as well.

@kevinrr888
Copy link
Member

kevinrr888 commented Dec 5, 2024

@cshannon - Yeah, I would be interested in looking at this. Currently working on some end of year stuff right now, so if you're free feel free to pick this up if you would like. I probably wouldn't be able to get to this until next week

@cshannon
Copy link
Contributor

cshannon commented Dec 5, 2024

@cshannon - Yeah, I would be interested in looking at this. Currently working on some end of year stuff right now, so if you're free feel free to pick this up if you would like. I probably wouldn't be able to get to this until next week

@kevinrr888 - Alright, I am planning to work on #5117 and to take a look into #5139 this week (and i will also look to see if there are any other issues that need to be worked on) so not sure yet if i I will have time either this week but i'll let you know if i start looking at it first

@kevinrr888
Copy link
Member

@cshannon - Cool, sounds good

@keith-turner
Copy link
Contributor

One other idea I thought of is if we could configure the fate thread pool be to use a PriorityBlockingQueue and then write a comparator to bump the priority of certain fate operation types higher than others so they run first.

A prioq would not work w/ current code structure. Currently fate has a single thread that scans looking for work. When it finds work it use a transfer queue to directly pass what it found to any of the idle fate threads that are looking for work. The code is structured this way to avoid queuing the same fate op over and over as the work finder thread keeps repeatedly scanning looking for work.

@cshannon
Copy link
Contributor

cshannon commented Dec 6, 2024

A prioq would not work w/ current code structure. Currently fate has a single thread that scans looking for work. When it finds work it use a transfer queue to directly pass what it found to any of the idle fate threads that are looking for work. The code is structured this way to avoid queuing the same fate op over and over as the work finder thread keeps repeatedly scanning looking for work.

I completely forgot that we changed that code to use a transfer queue so yeah that would definitely not work.

@cshannon
Copy link
Contributor

cshannon commented Dec 8, 2024

@kevinrr888 - I didn't get a chance to look at this so you can go ahead and grab it this week, I'll be working on #5014

@kevinrr888
Copy link
Member

@cshannon sounds good, thanks

@keith-turner
Copy link
Contributor

The current fate code creates two thread pools one for meta and one for user. Both thread pools give threads the same name in jstack. Would be nice if work around this gave any new threads pools different names in jstack.

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

No branches or pull requests

4 participants