-
Notifications
You must be signed in to change notification settings - Fork 13
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
Implement query throttling #485
Implement query throttling #485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Design self-review
await queryPermitter.Ask<QueryStartGranted>(RequestQueryStart.Instance); | ||
try | ||
{ | ||
return await factory.ExecuteWithTransactionAsync(level, token, handler); | ||
} | ||
finally | ||
{ | ||
queryPermitter.Tell(ReturnQueryStart.Instance); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the query throttling mechanism, we always ask for a permission before we execute any DB operations
await queryPermitter.Ask<QueryStartGranted>(RequestQueryStart.Instance); | ||
try | ||
{ | ||
return await factory.ConnectionFactory.ExecuteWithTransactionAsync(state, factory.IsolationLevel, factory.ShutdownToken, handler); | ||
} | ||
finally | ||
{ | ||
queryPermitter.Tell(ReturnQueryStart.Instance); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the query throttling mechanism, we always ask for a permission before we execute any DB operations
static async input => | ||
{ | ||
return await input._dbStateHolder.ExecuteWithTransactionAsync( | ||
return await input._dbStateHolder.ExecuteQueryWithTransactionAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace all ExecuteWithTransactionAsync()
method call inside the read journal with ExecuteQueryWithTransactionAsync()
to make sure that all DB queries are throttled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add query timeouts
@@ -142,7 +145,8 @@ public static AkkaConfigurationBuilder WithSqlPersistence( | |||
DatabaseMapping? databaseMapping = null, | |||
TagMode? tagStorageMode = null, | |||
bool? deleteCompatibilityMode = null, | |||
bool? useWriterUuidColumn = null) | |||
bool? useWriterUuidColumn = null, | |||
int? maxConcurrentQueries = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically a breaking change without a method overload, but I don't think there's any other meta-packages built on top of Akka.Persistence.Sql.Hosting so it's probably fine as it's source-compatible
CancellationToken token, | ||
Func<AkkaDataConnection, CancellationToken, Task<T>> handler) | ||
{ | ||
await queryPermitter.Ask<QueryStartGranted>(RequestQueryStart.Instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use a timeout or a CTS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
TState state, | ||
Func<AkkaDataConnection, CancellationToken, TState, Task<T>> handler) | ||
{ | ||
return factory.ConnectionFactory.ExecuteWithTransactionAsync(state, factory.IsolationLevel, factory.ShutdownToken, handler); | ||
await queryPermitter.Ask<QueryStartGranted>(RequestQueryStart.Instance); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use a timeout or a CTS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Implement query throttling scheme, similar to akkadotnet/akka.net#6436
Changes
akka.persistence.query.journal.sql.max-concurrent-queries
HOCON settingQueryThrottler
class