Skip to content
This repository was archived by the owner on Aug 4, 2022. It is now read-only.

[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

Open
singlis wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[REEF-2044] Serializing a List in Tang for C# has missing functionality #1479

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5ee47a
[REEF-2044] Serializing a List in Tang for C# has missing functionality
singlis Sep 12, 2018
0aaf08a
Merge remote-tracking branch 'origin/master' into REEF-2044
singlis Oct 16, 2018
354a880
Merge remote-tracking branch 'origin/master' into REEF-2044
singlis Oct 16, 2018
f847c75
- Added more unit tests, plus checks for null/empty strings in lists
singlis Oct 18, 2018
11b134d
- Updating to tests to handle the binding exception for null and emp…
singlis Oct 19, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 104 additions & 3 deletions lang/cs/Org.Apache.REEF.Tang.Tests/Configuration/TestConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
// specific language governing permissions and limitations
// under the License.

using System;
using System.Collections.Generic;
using Org.Apache.REEF.Common.Tasks;
using Org.Apache.REEF.Examples.Tasks.HelloTask;
using Org.Apache.REEF.Tang.Annotations;
Expand All @@ -31,6 +29,9 @@
using Org.Apache.REEF.Tang.Protobuf;
using Org.Apache.REEF.Tang.Tests.ScenarioTest;
using Org.Apache.REEF.Tang.Util;
using System;
using System.Collections.Generic;
using System.Linq;
using Xunit;

namespace Org.Apache.REEF.Tang.Tests.Configuration
Expand Down Expand Up @@ -425,6 +426,77 @@ public void TestTimerConfigurationWithClassHierarchy()
timer.sleep();
}

[Fact]
public void TestListConfig()
{
IList<string> stringList1 = new List<string>();
stringList1.Add("foo");
stringList1.Add("bar");
Copy link
Copy Markdown
Contributor

@motus motus Jan 5, 2019

Choose a reason for hiding this comment

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

Here and everywhere: can be simply

var stringList1 = new List<string> { "foo", "bar" };


IList<string> stringList2 = new List<string>();
stringList2.Add("test1");
stringList2.Add("test2");
stringList2.Add("test3");
stringList2.Add("test4");

IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList1)
.BindList<ListOfStrings2, string>(GenericType<ListOfStrings2>.Class, stringList2)
.Build();
Copy link
Copy Markdown
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

No need for the first argument in BindList(): that information is already passed as a generic parameter. can simply write

IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
    .BindList<ListOfStrings, string>(stringList1)
    .BindList<ListOfStrings2, string>(stringList2)
    .Build();

(also, maybe define static ITang Tang = TangFactory.GetTang(); that can be used in all tests?)


ConfigurationFile.WriteConfigurationFile(conf, "ListOfStringsConf.txt");
Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 9, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

This file should be deleted by the end of the tests. Or, better yet, serialize the configuration to a byte[] and back. #Resolved

Copy link
Copy Markdown
Contributor Author

@singlis singlis Oct 16, 2018

Choose a reason for hiding this comment

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

This is not needed -- I actually convert the config to a string and then read back in the string. So I have deleted this line.

In reply to: 223778711 [](ancestors = 223778711)


string s = ConfigurationFile.ToConfigurationString(conf);
IConfiguration conf2 = ConfigurationFile.GetConfiguration(s);

ListInjectTest injectTest = (ListInjectTest)TangFactory.GetTang().
NewInjector(conf2).GetInstance(typeof(ListInjectTest));
Copy link
Copy Markdown
Contributor

@motus motus Jan 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No need to cast - simply write

var injectTest = TangFactory.GetTang().NewInjector(conf2).GetInstance<ListInjectTest>();


Assert.Equal(stringList1, injectTest.List1);
Assert.Equal(stringList2, injectTest.List2);
}

[Fact]
public void TestListConfigWithAvroSerialization()
{
IList<string> stringList = new List<string>();
stringList.Add("foo");
stringList.Add("bar");
IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList)
.Build();

var serializer = new AvroConfigurationSerializer();
byte[] bytes = serializer.ToByteArray(conf);
IConfiguration conf2 = serializer.FromByteArray(bytes);
Assert.Equal(1, conf2.GetBoundList().Count);

var actualList = conf2.GetBoundList().First().Value;
Assert.Equal(stringList, actualList);
}

[Fact]
public void TestListSerializeNullStringValue()
Comment thread
singlis marked this conversation as resolved.
Outdated
{
string msg = null;
Copy link
Copy Markdown
Contributor

@motus motus Jan 7, 2019

Choose a reason for hiding this comment

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

msg is not used anywhere

IList<string> stringList = new List<string>();
stringList.Add(null);

try
{
IConfiguration conf = TangFactory.GetTang().NewConfigurationBuilder()
.BindList<ListOfStrings, string>(GenericType<ListOfStrings>.Class, stringList)
.Build();
var serializer = new AvroConfigurationSerializer();
byte[] bytes = serializer.ToByteArray(conf);
}
catch (IllegalStateException e)
{
msg = e.Message;
}
Assert.NotNull(msg);
}

[Fact]
public void TestSetConfig()
{
Expand All @@ -448,6 +520,7 @@ public void TestSetConfig()
Assert.True(actual.Contains("six"));
}


Copy link
Copy Markdown
Contributor

@motus motus Jan 5, 2019

Choose a reason for hiding this comment

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

why an extra empty line?

[Fact]
public void TestSetConfigWithAvroSerialization()
{
Expand All @@ -471,6 +544,7 @@ public void TestSetConfigWithAvroSerialization()
Assert.True(actual.Contains("six"));
}


[Fact]
public void TestNullStringValue()
{
Expand Down Expand Up @@ -508,11 +582,38 @@ public void TestSetConfigNullValue()
}
}

[NamedParameter]
class ListOfStrings : Name<IList<string>>
{

}

[NamedParameter]
class ListOfStrings2 : Name<IList<string>>
{

}

class ListInjectTest
{
public IList<string> List1;
public IList<string> List2;

[Inject]
ListInjectTest([Parameter(typeof(ListOfStrings))] IList<string> list1,
[Parameter(typeof(ListOfStrings2))] IList<string> list2)
{
this.List1 = list1;
this.List2 = list2;
}
}

[NamedParameter(DefaultValues = new string[] { "one", "two", "three" })]
class SetOfNumbers : Name<ISet<string>>
{
}


class Box
{
public ISet<string> Numbers;
Expand Down Expand Up @@ -560,4 +661,4 @@ public string GetString()
return str;
}
}
}
}
36 changes: 29 additions & 7 deletions lang/cs/Org.Apache.REEF.Tang/Formats/AvroConfigurationSerializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,6 @@
// specific language governing permissions and limitations
// under the License.

using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.Serialization;
using System.Text;
using Microsoft.Hadoop.Avro;
using Microsoft.Hadoop.Avro.Container;
using Newtonsoft.Json;
Expand All @@ -34,6 +27,13 @@
using Org.Apache.REEF.Tang.Types;
using Org.Apache.REEF.Tang.Util;
using Org.Apache.REEF.Utilities.Logging;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.Serialization;
using System.Text;

namespace Org.Apache.REEF.Tang.Formats
{
Expand Down Expand Up @@ -260,6 +260,28 @@ public AvroConfiguration ToAvroConfiguration(IConfiguration c)
l.Add(new ConfigurationEntry(e.Key.GetFullName(), val));
}

IEnumerator bl = conf.GetBoundList().GetEnumerator();
while (bl.MoveNext())
Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 9, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

why not a for each loop? #Resolved

Copy link
Copy Markdown
Contributor Author

@singlis singlis Oct 16, 2018

Choose a reason for hiding this comment

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

What is the expected behavior for an empty list? The code does run - however nothing is written out because there is nothing to serialize. If that is expected, I will modify the test accordingly.

In reply to: 223779318 [](ancestors = 223779318)

Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 16, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

Yes, I believe that is the expected behavior. #Resolved

{
KeyValuePair<INamedParameterNode, IList<object>> e = (KeyValuePair<INamedParameterNode, IList<object>>)bl.Current;
foreach (var item in e.Value)
{
string val = null;
if (item is string)
{
val = (string)item;
}
else if (item is INode)
{
val = ((INode)item).GetFullName();
}
else
{
Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);
Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 9, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

please don't use the Exceptions class anymore. Instead, just throw the exception. #Resolved

}
l.Add(new ConfigurationEntry(e.Key.GetFullName(), val));
}
}
Copy link
Copy Markdown
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

Also, maybe the whole loop can be expressed in Linq terms? e.g. along the lines of

l.UnionWith(conf.GetBoundList().SelectMany(kvp =>
{
    string key = kvp.Key.GetFullName();
    return kvp.Value.Select(item =>
    {
        if (item is string val) return new ConfigurationEntry(key, val);
        if (item is INode node) return new ConfigurationEntry(key, node.GetFullName());
        throw new TangApplicationException($"Unable to serialize list of type {item.GetType()}");
    });
}));

return new AvroConfiguration(Language.Cs.ToString(), l);
}

Expand Down
35 changes: 29 additions & 6 deletions lang/cs/Org.Apache.REEF.Tang/Formats/ConfigurationFile.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,19 @@
// specific language governing permissions and limitations
// under the License.

using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;
using Org.Apache.REEF.Tang.Exceptions;
using Org.Apache.REEF.Tang.Implementations.Configuration;
using Org.Apache.REEF.Tang.Implementations.Tang;
using Org.Apache.REEF.Tang.Interface;
using Org.Apache.REEF.Tang.Types;
using Org.Apache.REEF.Tang.Util;
using Org.Apache.REEF.Utilities.Logging;
using System;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Text;

namespace Org.Apache.REEF.Tang.Formats
{
Expand Down Expand Up @@ -139,6 +139,29 @@ public static HashSet<string> ToConfigurationStringList(IConfiguration c)
l.Add(GetFullName(e.Key) + '=' + Escape(val));
}

IEnumerator bl = conf.GetBoundList().GetEnumerator();
while (bl.MoveNext())
{
KeyValuePair<INamedParameterNode, IList<object>> e = (KeyValuePair<INamedParameterNode, IList<object>>)bl.Current;
foreach (var item in e.Value)
{
string val = null;
if (item is string)
{
val = GetFullName((string)item);
}
else if (e.Value is INode)
{
val = GetFullName((INode)e.Value);
}
else
{
Org.Apache.REEF.Utilities.Diagnostics.Exceptions.Throw(new IllegalStateException(), LOGGER);
Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 9, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

Same as above: Replace with throw. Also, add a message to the exception. #Resolved

}
l.Add(GetFullName(e.Key) + '=' + Escape(val));
}
}

return l;
}

Expand Down
15 changes: 15 additions & 0 deletions lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationBuilderImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,10 @@ public void BindParameter(INamedParameterNode name, string value)
{
BindSetEntry((INamedParameterNode)name, value);
}
else if (name.IsList())
{
BindList((INamedParameterNode)name, value);
}
else
{
try
Expand All @@ -289,6 +293,17 @@ public void BindParameter(INamedParameterNode name, string value)
}
}

public void BindList(INamedParameterNode iface, string impl)
{
IList<object> l;
if (!BoundLists.TryGetValue(iface, out l))
Copy link
Copy Markdown
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

can join these two lines, i.e.

if (!BoundLists.TryGetValue(iface, out IList<object> l))

etc.

{
l = new List<object>();
BoundLists.Add(iface, l);
}
l.Add((object)impl);
Copy link
Copy Markdown
Contributor

@motus motus Jan 8, 2019

Choose a reason for hiding this comment

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

no need to cast

}

public void BindImplementation(IClassNode n, IClassNode m)
{
if (this.ClassHierarchy.IsImplementation(n, m))
Expand Down
10 changes: 5 additions & 5 deletions lang/cs/Org.Apache.REEF.Tang/Implementations/Configuration/ConfigurationImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@
// specific language governing permissions and limitations
// under the License.

using System.Collections.Generic;
using Org.Apache.REEF.Tang.Interface;
using Org.Apache.REEF.Tang.Types;
using System.Collections.Generic;

namespace Org.Apache.REEF.Tang.Implementations.Configuration
Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 9, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

Changes in this file seem superficial only. Maybe revert? #Resolved

Copy link
Copy Markdown
Contributor Author

@singlis singlis Oct 16, 2018

Choose a reason for hiding this comment

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

It was sorting the using - but there are no additional changes so I dont think its justified. I will revert.

In reply to: 223779942 [](ancestors = 223779942)

{
Expand All @@ -39,7 +39,7 @@ public IConfigurationBuilder newBuilder()
{
return ((ConfigurationImpl)Builder.Build()).Builder;
}

public ICollection<IClassNode> GetBoundImplementations()
{
return Builder.BoundImpls.Keys;
Expand Down Expand Up @@ -95,12 +95,12 @@ public ICollection<IClassNode> GetLegacyConstructors()
return Builder.LegacyConstructors.Keys;
}

public ISet<object> GetBoundSet(INamedParameterNode np)
public ISet<object> GetBoundSet(INamedParameterNode np)
{
return Builder.BoundSetEntries.GetValuesForKey(np);
}

public IEnumerator<KeyValuePair<INamedParameterNode, object>> GetBoundSets()
public IEnumerator<KeyValuePair<INamedParameterNode, object>> GetBoundSets()
{
return Builder.BoundSetEntries.GetEnumerator();
}
Expand All @@ -117,4 +117,4 @@ public IList<object> GetBoundList(INamedParameterNode np)
return list;
}
}
}
}
1 change: 1 addition & 0 deletions lang/cs/Org.Apache.REEF.Tang/Interface/IConfiguration.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ public interface IConfiguration
ICollection<IClassNode> GetLegacyConstructors();

IEnumerator<KeyValuePair<INamedParameterNode, object>> GetBoundSets();

Copy link
Copy Markdown
Contributor

@markusweimer markusweimer Oct 9, 2018 •
edited by singlis
Loading

Choose a reason for hiding this comment

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

Changes in this file are superficial only. Please revert. #Resolved

IDictionary<INamedParameterNode, IList<object>> GetBoundList();
}
}