Fix for Issue #2443 #2459
Fix for Issue #2443 #2459
Conversation
|
This pr only addresses: #2443 However i noticed the other proto generated classes also, aren't in their respective namespaces. |
|
Needs some more work to fix build issues. |
|
@Danthar need to approve the updated API for Akka.Remote |
| @@ -8,959 +8,6 @@ | |||
| [assembly: System.Runtime.InteropServices.ComVisibleAttribute(false)] | |||
| [assembly: System.Runtime.InteropServices.GuidAttribute("78986bdb-73f7-4532-8e03-1c9ccbe8148e")] | |||
| [assembly: System.Runtime.Versioning.TargetFrameworkAttribute(".NETFramework,Version=v4.5", FrameworkDisplayName=".NET Framework 4.5")] | |||
| public sealed class AckAndEnvelopeContainer : Google.ProtocolBuffers.GeneratedMessage<AckAndEnvelopeContainer, AckAndEnvelopeContainer.Builder> | |||
There was a problem hiding this comment.
While I think this change is good, we need to check that this doesn't break the wire format with versions of Akka.NET 1.1.3 and earlier
There was a problem hiding this comment.
uhm, your comparing the wrong side here?
| namespace Akka.Remote.Proto | ||
| { | ||
| public sealed class AckAndEnvelopeContainer : Google.ProtocolBuffers.GeneratedMessage<Akka.Remote.Proto.AckAndEnvelopeContainer, Akka.Remote.Proto.AckAndEnvelopeContainer.Builder> | ||
| { |
There was a problem hiding this comment.
This part is current code.
As you can see, its still the same. Just wrapped in an namespace.
However because if the difference in layout. The entire file became hard to compare. Which is also why the api approval changes are huge.
|
I do understand your concerns though @Aaronontheweb . However short of manually lining up the ApiApproval before and after files. It seems like something hard to verify. The main problem is probably, that the newer version of protobuf used, generated a different layout. Which makes comparing the changes hard. Im also open to other suggestions. |
|
@Danthar run one actorsystem using the latest from NuGet and have it message a second instance created from your local branch with the protobuf changes over Akka.Remote. Only sure way to know. |
|
Yeah I realized that. However I dismissed that option, because I feel like its not an exhaustive enough test ? Either way, ill rebuild the ChatServer example for both versions of Akka and test with that. |
|
Ok. I manually verified that 1.1.3 clients can connect and communicate with a client running the latest changes. And vice versa. It all worked like a charm. 👍 |
… made it internal.
… and made the types internal
…es in other assemblies as well.
|
Cool, I'm satisfied then! |
|
Tested this locally too. Was able to join a lighthouse cluster running Akka 1.1.3 using this branch, including the dotnetty transports!!! |
I noticed that the resulting layout of the files are very different. Since the proto files are unchanged. It should not really matter.