-
Notifications
You must be signed in to change notification settings - Fork 454
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
Improve performance of seeding split fate operations #5160
Comments
Looked into how this could be done and came up with one possible way that seems like it could break into two PRs. First could modify FateStore to remove the following method. Optional<FateId> seedTransaction(Fate.FateOperation txName, FateKey fateKey, Repo<T> repo,
boolean autoCleanUp); and replace it with the following interface Seeder<T> extends AutoCloseable {
// For UserFateStore this would create a conditional mutation and add it to the conditional writer
void attemptToSeedTransaction(Fate.FateOperation txName, FateKey fateKey, Repo<T> repo,
boolean autoCleanUp);
// This would check the status of all added conditional mutations, retry unknown, and then close the conditional writer.
void close();
}
// Creates a conditional writer for the user fate store. For Zookeeper all this code will probably do the same thing its currently doing as zookeeper does not support multi-node operations.
public Seeder<T> beginSeeding(); The current code in the manager could be left as is to be changed in a second PR. The way that code is structured it only knows about a single thing to seed at a time, so it would go through the three step process with a single thing and have the same behavior until the second PR is done. In a second PR the manager code could be modified from adding single task to a thread pool to instead add task to a queue and have a single background thread that takes batches of task of the thread pool and uses the new fate store API to write them all at once. There are a few different way this could be done. Would be nice to preserve the current behavior of not concurrently seeding the same thing in order to avoid unnecessary work. |
I am currently working on the last PR and changes for #5014 (Adding in a new thread and Fate operations to handle automatic merges similar to automatic splits), but when I am done if no one else picks this up I would be interested in taking a look. However, if someone else wants to work on it first that is fine as I'm not quire sure how much time it will take for #5014 to finish yet. i'll respond back and assign myself when i have time to work on it if no one else does. |
I went ahead and assigned myself to take a look. |
This adds a new Seeder API that can be used to seed transactions with a FateKey. The purpose of this new API is to allow for eventual batching of multiple seeding attempts together into one conditional mutation to improve performance. This change just updates the api and all calls to the seeder to attempt seeding will be performed individually. A future update will support the new functionality of batching and commiting all the changes together when the Seeder is closed. This change is for Part 1 of apache#5160
This adds a new Seeder API that can be used to seed transactions with a FateKey. The purpose of this new API is to allow for eventual batching of multiple seeding attempts together into one conditional mutation to improve performance. This change just updates the api and all calls to the seeder to attempt seeding will be performed individually. A future update will support the new functionality of batching and commiting all the changes together when the Seeder is closed. This change is for Part 1 of apache#5160
This adds a new Seeder API that can be used to seed transactions with a FateKey. The purpose of this new API is to allow for eventual batching of multiple seeding attempts together into one conditional mutation to improve performance. This change just updates the api and all calls to the seeder to attempt seeding will be performed individually. A future update will support the new functionality of batching and commiting all the changes together when the Seeder is closed. This change is for Part 1 of apache#5160
Is your feature request related to a problem? Please describe.
The changes in #5122 improved the performance of seeding fate split operations. However there is still further room for improvement. This code will seed a single split operation which will write a single conditional mutation. Writing single conditional mutations at a time requires waiting on them to committed in the write ahead log. Could get much better performance by writing many conditional mutaitons at once as this would allow many to be written to the write ahead log and then wait for all of them to be committed.
Describe the solution you'd like
The code for seeding split fate operations is restructured so that is takes a batch of tablets that need split operations seeded and writes the entire batch to a conditional writer. This would avoid the single writes. The current code avoids two problems and we would want to continue avoiding these when restructuring the code.
The first problem the current code avoids is attempting to seed the same tablet for split when its currently in the processes of seeding. This can happen when there is a situation like 100 tablets that need to split and tablet group watcher keeps finding these and queueing them up for seeding over and over. This will not cause problems with correctness, it just waste resources.
The second problem the current code avoids is making the tablet group watcher wait on seeding split operations. If there is a problem writing to the fate table to seed split, this will not block the tablet group watcher. Instead it eventually starts dropping/ignoring request to seed split operations.
The text was updated successfully, but these errors were encountered: