Prepare ClusterTools API to the stable version #2561
Conversation
| @@ -62,44 +62,44 @@ public static ClusterClientSettings Create(Config config) | |||
| /// <summary> | |||
| /// Actor paths of the <see cref="ClusterReceptionist"/> actors on the servers (cluster nodes) that the client will try to contact initially. | |||
| /// </summary> | |||
| public readonly IImmutableSet<ActorPath> InitialContacts; | |||
| public IImmutableSet<ActorPath> InitialContacts { get; } | |||
There was a problem hiding this comment.
Should we expose ImmutableList instead of some ReadOnly abstraction?
There was a problem hiding this comment.
Exposing the interface is fine imo.
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// TBD | ||
| /// </summary> | ||
| [Serializable] | ||
| public sealed class GetTopics : IEquatable<GetTopics> |
There was a problem hiding this comment.
Why we need an IEquatable for singletons?
There was a problem hiding this comment.
This is serializable class. Singleton messages are not guaranteed to be deserialized back into Instance object. Moreover, usually they are guaranteed to be different objects.
There was a problem hiding this comment.
We don't transfer this object via wire. it is local only
| public CurrentTopics(string[] topics) { } | ||
| public string[] Topics { get; } |
There was a problem hiding this comment.
I'm not fan of exposing arrays in public APIs. Could I change it to IReadOnlyCollection ?
There was a problem hiding this comment.
In this case I do not think it matters much. Wrapping it in an IReadOnlyCollection in this case only provides an illusion of safety.
|
Another note that I want to add. Im not a fan of forcing people to define an We could still provide a private copy constructor that does accept the |
|
@alexvaluyskiy minor merge conflict |
a0d1ab6 to
ca0d3b5
Compare
|
@Danthar @alexvaluyskiy @Horusiath is this ready for merge? |
|
@Aaronontheweb ready |
Uh oh!
There was an error while loading. Please reload this page.