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

#4353 Config.WithFallback is acting inconsistently #4358

146 changes: 146 additions & 0 deletions src/core/Akka.Cluster.Tests.MultiNode/Bugfix4353Specs.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
using System;
using System.Linq;
using System.Collections.Generic;
using System.Threading;

using Akka.Actor;
using Akka.Cluster.TestKit;
using Akka.Configuration;
using Akka.Remote.TestKit;
using Akka.TestKit;

namespace Akka.Cluster.Tests.MultiNode
{
public class Bugfix4353Spec : MultiNodeClusterSpec
{
protected RoleName First;
protected RoleName Second;
protected RoleName Third;

public Bugfix4353Spec() : this(new Bugfix4353SpecsConfig())
{
}

protected Bugfix4353Spec(Bugfix4353SpecsConfig config) : base(config, typeof(Bugfix4353Spec))
{
First = config.First;
Second = config.Second;
Third = config.Third;
}

[MultiNodeFact]
public void Bugfix4353Spec_Cluster_of_3_must_reach_cnovergence()
{
AwaitClusterUp(First, Second, Third);
EnterBarrier("after-1");
}
}

public class Bugfix4353SpecsConfig : MultiNodeConfig
{
private static readonly string[] Hocons =
{
@"akka : {
actor : {
provider : cluster
}
}",

@"akka : {
stdout-loglevel : INFO
loglevel : INFO
log-config-on-start : on
loggers : [""Akka.Event.DefaultLogger""],
actor : {
debug : {
receive : on
autoreceive : on
lifecycle : on
event-stream : on
unhandled : on
}
}
}",

@"akka : {
remote : {
dot-netty : {
tcp : {
log-transport : true
transport-class : ""Akka.Remote.Transport.DotNetty.TcpTransport, Akka.Remote""
transport-protocol : tcp
hostname : 0.0.0.0
public-hostname : localhost
}
}
}
}",

@"akka : {
cluster : {
log-info : on
seed-nodes : [
""akka.tcp://Bugfix4353Spec@localhost:5001"",
""akka.tcp://Bugfix4353Spec@localhost:5002"",
""akka.tcp://Bugfix4353Spec@localhost:5003""
]
roles : [seed]
role : { }
}
}"
};

public static Config Config
{
get
{
var config = ConfigurationFactory.Empty;

foreach (var hocon in Hocons)
{
config = config.WithFallback(ConfigurationFactory.ParseString(hocon));
}
return config;
}
}

public readonly RoleName First;
public readonly RoleName Second;
public readonly RoleName Third;

public Bugfix4353SpecsConfig()
{

First = Role("first");
Second = Role("second");
Third = Role("third");

CommonConfig = MultiNodeClusterSpec.ClusterConfig(false);

NodeConfig(
new[] { First },
new[] {
ConfigurationFactory
.ParseString("akka.remote.dot-netty.tcp.port : 5001")
.WithFallback(Config)
});

NodeConfig(
new[] { Second },
new[] {
ConfigurationFactory
.ParseString("akka.remote.dot-netty.tcp.port : 5002")
.WithFallback(Config)
});

NodeConfig(
new[] { Third },
new[] {
ConfigurationFactory
.ParseString("akka.remote.dot-netty.tcp.port : 5003")
.WithFallback(Config)
});

}
}
}
4 changes: 4 additions & 0 deletions src/core/Akka/Configuration/Hocon/HoconObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,8 @@ public void Merge(HoconObject other)
//if both values are objects, merge them
if (thisItem.IsObject() && otherItem.Value.IsObject())
thisItem.GetObject().Merge(otherItem.Value.GetObject());
else
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, so if I'm reading this right, if both items are objects, they are to be merged, but how is the alternative supposed to work? what is otherItem's type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HOCON have 3 basic datatypes, Object, Array, and Literal.
Array and Literal will always override merged object fields, but when an Object met another Object, they got merged instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The full specification for HOCON can be read here: https://github.com/akkadotnet/hocon

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, I see it now

Copy link
Contributor

Choose a reason for hiding this comment

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

let's give it a shot then

modified.Add(new KeyValuePair<string, HoconValue>(otherItem.Key, otherItem.Value));
}
else
{
Expand Down Expand Up @@ -228,6 +230,8 @@ internal HoconObject MergeImmutable(HoconObject other)
mergedValue.AppendValue(mergedObject);
thisItems[otherItem.Key] = mergedValue;
}
else
thisItems[otherItem.Key] = otherItem.Value;
Copy link
Member

Choose a reason for hiding this comment

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

So for non-HoconObject types, we just ignored them on merge before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it causes a lot of missing object fields, I'm surprised that this bug went through the test suite

Copy link
Member

Choose a reason for hiding this comment

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

Is there a simple test case we can add for this to the HOCON test suite? Thinking about it mostly as a regression test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do have rigorous testing suite in the standalone HOCON project, I think we used to have a limited HOCON test suite in Akka, they might be accidentally removed.

}
else
{
Expand Down