#3422 SQL persistence providers: Data reader SequentialAccess #3429
Conversation
| /// <summary> | ||
| /// Uses the CommandBehavior.SequentialAccess when creating the command, providing a performance improvement for reading large BLOBS. | ||
| /// </summary> | ||
| public readonly bool UseSequentialAccess; |
There was a problem hiding this comment.
We use auto-get properties.
| using (var reader = await command.ExecuteReaderAsync(cancellationToken)) | ||
| var commandBehavior = CommandBehavior.Default; | ||
|
|
||
| if (Configuration.UseSequentialAccess) |
There was a problem hiding this comment.
We can use if/else just to avoid double assignment and preserve intent.
| using (var reader = await command.ExecuteReaderAsync(cancellationToken)) | ||
| var commandBehavior = CommandBehavior.Default; | ||
|
|
||
| if (Configuration.UseSequentialAccess) |
There was a problem hiding this comment.
We can use if/else just to avoid double assignment and preserve intent.
| /// <summary> | ||
| /// Uses the CommandBehavior.SequentialAccess when creating the command, providing a performance improvement for reading large BLOBS. | ||
| /// </summary> | ||
| public readonly bool UseSequentialAccess; |
There was a problem hiding this comment.
Please use auto-get property.
|
|
||
| var commandBehavior = CommandBehavior.Default; | ||
|
|
||
| if (Configuration.UseSequentialAccess) |
There was a problem hiding this comment.
We can use if/else just to avoid double assignment and preserve intent.
|
Thanks @Horusiath for the quick code review. I updated according to you suggestions. |
…s can use Sequential access, which improves reading speed for large payloads. - Added for Snapshot query - Added for journal queries This change is not backwards compatible
|
Related issue: #3422 |
| @@ -147,6 +154,7 @@ public class QueryConfiguration | |||
| SerializerIdColumnName = serializerIdColumnName; | |||
| Timeout = timeout; | |||
| DefaultSerializer = defaultSerializer; | |||
| UseSequentialAccess = useSequentialAccess; | |||
There was a problem hiding this comment.
What's the default value of this? Is it left up to the plugin implementation? Should we add it to some of the built-in HOCON files?
There was a problem hiding this comment.
Sorry for the late response.
Default value is false = current behavior
As the configuration is read by the plugin implementations it is basically up to the plugins. Therefore I'm not sure if it makes sense to put into some HOCO file on Akka level.
So I'm not sure if further changes are needed.
There was a problem hiding this comment.
Sorry for the late response.
Default value is false = current behavior
As the configuration is read by the plugin implementations it is basically up to the plugins. Therefore I'm not sure if it makes sense to put into some HOCO file on Akka level.
So I'm not sure if further changes are needed.
|
@TietoOliverKurowski did you see my comments? |
|
Ok, got it - so this is a breaking change from an API perspective and will require us to re-compile any Akka.Persistence plugins that use SQL. Not the end of the world. We'll go ahead and do that. |
|
Great, thanks a lot! |
Configuration option for SQL persistence providers: Data readers can use Sequential access, which improves reading speed for large payloads.
This change is not backwards compatible.
As far as I can see there are no tests for the Akka.Persistence.SQL.Common project, so I didn't add any new test.
I also prepared a change for the SQL Server provider, but I wanted to wait for your review comments before publishing it.
Please let me know of any thinks I still need to do. This is my first pull request, so everything is quite new ;)