Skip to content
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

Fix DData infinite status/gossip round #5056

Merged
merged 6 commits into from Jun 2, 2021

Conversation

Arkatufus
Copy link
Contributor

Closes #5022

var path = _config.GetString("dir");
_dir = path.EndsWith(DatabaseName)
? Path.GetFullPath($"{path}-{Context.System.Name}-{Self.Path.Parent.Name}-{Cluster.Cluster.Get(Context.System).SelfAddress.Port}")
: Path.GetFullPath(path);

_dirExists = Directory.Exists(_dir);

_log.Info($"Using durable data in LMDB directory [{_dir}]");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move logging here, should be called once at startup, not every time a new LMDB environment is created


namespace Akka.DistributedData.Serialization.Proto.Msg
{
internal sealed partial class OtherMessage : IComparable<OtherMessage>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to allow OtherMessage can be sorted

Copy link
Member

Choose a reason for hiding this comment

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

Would it be easier to just write an IComparer? So we don't have to hack around a generated class?

b.Vvector = SerializationSupport.VersionVectorToProto(ints.VersionVector);
b.TypeInfo.Type = ValType.Int;
var intElements = new List<int>(ints.ElementsMap.Keys);
intElements.Sort();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ORSet elements need to be sorted during serialization so that it have consistent SHA-1 digest

foreach (var val in intElements)
{
b.IntElements.Add(val);
b.Dots.Add(SerializationSupport.VersionVectorToProto(ints.ElementsMap[val]));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ElementsMap isn't serialized as a dictionary, instead keys and version vector are serialized as separate lists. Due to this serialization scheme, key ordering and value ordering matters. VersionVector need to be inserted in the same order as the newly sorted keys.

@Arkatufus Arkatufus marked this pull request as ready for review June 1, 2021 20:17
@Aaronontheweb Aaronontheweb self-requested a review June 1, 2021 20:25
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.

One small change

@@ -80,11 +82,16 @@ public LmdbDurableStore(Config config)
TimeSpan.Zero :
_config.GetTimeSpan("write-behind-interval");

_mapSize = _config.GetByteSize("map-size") ?? 100 * 1024 * 1024;
Copy link
Member

Choose a reason for hiding this comment

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

This was always the default size, so no big deal.


private readonly TimeSpan _writeBehindInterval;
private readonly string _dir;
private bool _dirExists;
Copy link
Member

Choose a reason for hiding this comment

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

Good idea to cache this


namespace Akka.DistributedData.Serialization.Proto.Msg
{
internal sealed partial class OtherMessage : IComparable<OtherMessage>
Copy link
Member

Choose a reason for hiding this comment

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

Would it be easier to just write an IComparer? So we don't have to hack around a generated class?

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.

LGTM


namespace Akka.DistributedData.Serialization
{
internal class OtherMessageComparer : IComparer<OtherMessage>
Copy link
Member

Choose a reason for hiding this comment

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

We should make this static and have a readonly singleton here, but we can easily do that in an update

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you already did that inside the serializer

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) June 2, 2021 01:24
@Aaronontheweb Aaronontheweb merged commit 1bec20a into akkadotnet:dev Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Akka.DistributedData: memory leak when recovering events from LMDB data store
2 participants