Skip to content

Fix selecting serializer in BatchingSqlJournal #2946

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 9, 2017
Merged

Fix selecting serializer in BatchingSqlJournal #2946

merged 3 commits into from
Aug 9, 2017

Conversation

pmbanka
Copy link
Contributor

@pmbanka pmbanka commented Aug 9, 2017

BatchingSqlJournal has a bug where it selects serializer for a payload using Serialization.FindSerializerFor method, but instead of passing a payload into the method, it passes the type of payload.

I changed the type of _getSerializer because I believe it will prevent making similar mistakes in the future.

@Aaronontheweb
Copy link
Member

@heynickc @Arkatufus

- AllTests
[14:17:37]	  - RunTestsNetCore
[14:17:37]	No target was successfully completed
[14:17:37]	System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation. ---> System.Exception: Target "MultiNodeTestsNetCore" is not defined.
[14:17:37]	   at Fake.TargetHelper.getTarget@77-2.Invoke(String message) in C:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 77
[14:17:37]	   at Fake.TargetHelper.visitDependenciesAux@344[a](FSharpFunc`2 fGetDependencies, FSharpFunc`2 fVisit, HashSet`1 visited, List`1 ordered, Int32 level, Tuple`2 tupledArg) in C:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 345
[14:17:37]	   at Fake.TargetHelper.visit@341-1[a](FSharpFunc`2 fGetDependencies, FSharpFunc`2 fVisit, String targetName) in C:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 351
[14:17:37]	   at Fake.TargetHelper.visitDependencies(FSharpFunc`2 fVisit, String targetName) in C:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 355
[14:17:37]	   at Fake.TargetHelper.run$cont@502(String targetName, Unit unitVar) in C:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 538
[14:17:37]	   at Fake.TargetHelper.run(String targetName) in C:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 502
[14:17:37]	   at Fake.AdditionalSyntax.RunTargetOrDefault(String defaultTarget) in C:\code\FAKE\src\app\FakeLib\AdditionalSyntax.fs:line 29
[14:17:37]	   at <StartupCode$FSI_0005>.$FSI_0005_Build$fsx.main@()
[14:17:37]	   --- End of inner exception stack trace ---
[14:17:37]	   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
[14:17:37]	   at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
[14:17:37]	   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
[14:17:37]	   at System.RuntimeType.InvokeMember(String name, BindingFlags bindingFlags, Binder binder, Object target, Object[] providedArgs, ParameterModifier[] modifiers, CultureInfo culture, String[] namedParams)
[14:17:37]	   at Fake.FSIHelper.runFAKEScriptWithFsiArgsAndRedirectMessages@343.Invoke(Unit unitVar0) in C:\code\FAKE\src\app\FakeLib\FSIHelper.fs:line 349
[14:17:37]	
[14:17:37]	Process exited with code 1

MNTR on .NET Core issue

@heynickc
Copy link
Contributor

heynickc commented Aug 9, 2017

@Aaronontheweb @pmbanka this PR is targeting dev branch. should it be targeting v1.3 branch?

@heynickc
Copy link
Contributor

heynickc commented Aug 9, 2017

@Aaronontheweb @pmbanka if it is supposed to be targeting dev branch, I'll need to make a quick change to the dev branch build scripts to no-op on this build step. stand by and I will submit a PR, it'll need to be there anyway

@pmbanka
Copy link
Contributor Author

pmbanka commented Aug 9, 2017 •
edited
Loading

Update - you can disregard whatever is below the line, actually old journal will also throw NRE on a null payload, so this is at least consistent.

@Aaronontheweb @heynickc one more thing. The batching version is not 100% replacement of non-batching event journal, because regular SqlJournal calls FindSerializerFor, which handles null payload:

akka.net/src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/QueryExecutor.cs

Line 623 in 020ae2d

var serializer = Serialization.FindSerializerFor(e.Payload);

whereas current implementation of BatchingSqlJournal will throw NRE when passed a message with null payload (because of a GetType call). Is it a bug?

@pmbanka
Copy link
Contributor Author

pmbanka commented Aug 9, 2017

@heynickc I targeted dev only because this is what contribution guidelines recommend.

@Aaronontheweb
Copy link
Member

@pmbanka target the v1.3 branch. We're releasing it in a day or so.

@pmbanka pmbanka changed the base branch from dev to v1.3 August 9, 2017 15:01
Aaronontheweb
Aaronontheweb approved these changes Aug 9, 2017
View reviewed changes
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Looks good to me.

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs
@@ -527,7 +526,7 @@ protected BatchingSqlJournal(BatchingSqlJournalSetup setup)

_remainingOperations = Setup.MaxConcurrentOperations;
Buffer = new Queue<IJournalRequest>(Setup.MaxBatchSize);
_getSerializer = Context.System.Serialization.FindSerializerFor;
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch

src/contrib/persistence/Akka.Persistence.Sql.Common/Journal/BatchingSqlJournal.cs
@@ -1151,7 +1150,7 @@ protected virtual void WriteEvent(TCommand command, IPersistentRepresentation pe
var manifest = string.IsNullOrEmpty(persistent.Manifest)
? payloadType.TypeQualifiedName()
: persistent.Manifest;
var serializer = _getSerializer(payloadType);
var serializer = _serialization.FindSerializerForType(payloadType);
Copy link
Member

Choose a reason for hiding this comment

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

Looks right to me.

@Aaronontheweb
Copy link
Member

cc @alexvaluyskiy your thoughts?

@Aaronontheweb Aaronontheweb added this to the 1.3.0 milestone Aug 9, 2017
@heynickc heynickc merged commit 69479d8 into akkadotnet:v1.3 Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaronontheweb Aaronontheweb Aaronontheweb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@pmbanka @Aaronontheweb @heynickc