GrainDirectory Refactoring and Multi-Cluster Registration #1108

sebastianburckhardt · 2015-12-03T20:33:30Z

This branch contains

some basic refactoring of the local and remote grain directory. There was a lot of code duplication between the various functions (register, registersingleact, unregister, delete) and between the local and remote grain directory. The proposed refactoring puts shared functionality into LocalGrainDirectory. It adds a hopcount parameter into the signature of the directory operations (replacing the retries parameter), which allows us to tell if something is a remote or local request, and whether we should retry.
hooks for specifying a multi-cluster grain registration strategy. This strategy is defined by the type of the grain (e.g. using attributes). The strategy is used to look up the corresponding registrar for the grain. The actual registration/unregistration/deletion of a grain is done by that registrar.

dnfclas · 2015-12-03T21:39:37Z

@sebastianburckhardt, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

jdom · 2015-12-03T22:36:34Z

src/OrleansRuntime/Messaging/SiloMessageSender.cs

        private const int DEFAULT_MAX_RETRIES = 0;
-        private readonly Dictionary<SiloAddress, DateTime> lastConnectionFailure;
+        protected readonly Dictionary<SiloAddress, DateTime> lastConnectionFailure;


what's the goal in making these fields protected? I couldn't find any usages of them outside this class (and also I don't see any subclasses of SiloMessageSender.

Good catch, I agree it should be private. I think this was just left over from some code that no longer exists.

gabikliot · 2015-12-04T05:48:12Z

src/Orleans/IDs/ActivationAddress.cs

@@ -32,6 +33,7 @@ internal class ActivationAddress
        public GrainId Grain { get; private set; }
        public ActivationId Activation { get; private set; }
        public SiloAddress Silo { get; private set; }
+        public MultiClusterStatus Status { get; private set; }


I think you now also need to change the way ActivationAddress is serialized in Orleans serializer. We have manually written built in serializer for all runtime types. Look at BuiltInTypes.

Unless of course you explicitly do not want this extra field to be serialized, and then you need to mark it as NonSerialized.

Good point. I am not quite sure. I think it is only used locally, but I will take a look.

There seems to be at least one case where the status has to be serialized (namely, when handing off activations). I have updated the binarystreamreader and binarystreamwriter and the serialization unit test accordingly. This adds one byte.

… to singletonSystemTargetNames

gabikliot · 2015-12-22T18:08:27Z

Regarding your question about AddressesAndTag:

mark it Immutable, since it is.
can leave Serailizable. It is not required, but specifies intent.
Check the output of the build. It prints which serializers were generated. This one should be, since the type appears in the system target method signature. But worth double checking. If it does not, we can/should fix that.

jdom · 2016-01-13T22:45:54Z

src/OrleansRuntime/GrainDirectory/GrainDirectoryHandoffManager.cs

                {
                    // Force the list to be created in order to avoid race conditions
-                    return result.Item1.Select(pair => ActivationAddress.GetAddress(pair.Item1, grain, pair.Item2)).ToList();
+                    return result.Addresses.Select(a => ActivationAddress.GetAddress(a.Silo, grain, a.Activation, a.Status)).ToList();


Any reason to recreate the list? It doesn't seem to be accomplishing anything. It creates an identical List<ActivationAddress> as result.ActivationAddresses

I agree that this is unnecessary, since an identical list is now being constructed in partition.LooupGrain and can be used directly.

Also, the comment about race conditions seems irrelevant since the list returned by partition.LookupGrain is always fresh, and the ActivationAddress objects are immutable.

I will simplify this.

sergeybykov · 2016-01-28T17:08:08Z

src/OrleansRuntime/GrainDirectory/LocalGrainDirectory.cs

-                RegistrationsLocal.Increment();
-                // if I am the owner, store the new activation locally
-                DirectoryPartition.AddActivation(address.Grain, address.Activation, address.Silo);
+                (singleActivation ? RegistrationsSingleActLocal : RegistrationsLocal).Increment();


The naming is a bit confusing to me - it is easy to read it as if RegistrationsLocal is all local registration including single-activation ones while in reality it seems that it actually means RegistrationsMultipleLocal or something like that.

I agree with you, but did not dare to change the counter names since that could break someone else's stuff.

sergeybykov · 2016-01-29T00:19:33Z

I think I'm done with my review pass. Once the outstanding comments are addressed, I think we can rebase, squash, retest, and merge.

sebastianburckhardt · 2016-01-29T17:57:09Z

I think I have addressed all suggestions and questions. I will wait a couple hours and then squash, unless there are objections.

sergeybykov · 2016-01-29T18:19:41Z

@sebastianburckhardt Please give us some more time. I'm in a meeting unable to do much. I think give us till the rest of the day before rebasing and squashing. Thanks.

sergeybykov · 2016-01-30T01:40:10Z

src/OrleansRuntime/GrainDirectory/RemoteGrainDirectory.cs

-            return Task.WhenAll(addresses.Select(addr => Register(addr, retries)));
-        }
+            if (addresses == null || addresses.Count == 0)
+                throw new ArgumentException("addresses");


A nit: ArgumentException takes an error message as a constructor argument, e.g. "addresses cannot be an empty list or null" and an optional parameter name. ArgumentNullException OTOH takes just an argument name.

ok. I will change that and then rebase and squash.

sergeybykov · 2016-01-30T01:40:51Z

I am ready for rebasing and squashing of this.

sebastianburckhardt · 2016-02-01T17:32:53Z

Rebased and squashed.

jdom · 2016-02-01T17:46:04Z

Awesome, thanks everybody. Doing some final test runs on our VSO and will get this merged by EOD

GrainDirectory Refactoring and Multi-Cluster Registration

jdom · 2016-02-02T01:33:12Z

Alright, this is in! Load & Reliability tests worked fine. Thanks to all involved in this implementation and review 😄

sergeybykov · 2016-02-02T04:04:02Z

A huge leap into geo-distribution has begun! :-)

gabikliot · 2016-02-02T04:44:35Z

👍

jthelin · 2016-02-02T16:40:39Z

Whoo. Nice job @sebastianburckhardt and everyone.
One small step for Orleans ....
The future is Geo!

dnfclas added the cla-required label Dec 3, 2015

gabikliot changed the title ~~add multi-cluster registration hooks; refactor local/remote grain dir…~~ add multi-cluster registration hooks; refactor local/remote grain directory functions (Register, RegisterSingleAct, Unregister, Delete) to reduce code duplication Dec 3, 2015

sebastianburckhardt mentioned this pull request Dec 3, 2015

Geo-Distribution Project #948

Closed

sebastianburckhardt changed the title ~~add multi-cluster registration hooks; refactor local/remote grain directory functions (Register, RegisterSingleAct, Unregister, Delete) to reduce code duplication~~ GrainDirectory Refactoring and Multi-Cluster Registration Dec 3, 2015

dnfclas added cla-signed and removed cla-required labels Dec 3, 2015

gabikliot added this to the Multi-Cluster milestone Dec 3, 2015

jdom reviewed Dec 3, 2015
View reviewed changes

jthelin added the enhancement label Dec 3, 2015

gabikliot reviewed Dec 4, 2015
View reviewed changes

sebastianburckhardt added a commit to sebastianburckhardt/orleans that referenced this pull request Dec 4, 2015

fix comments as discussed in dotnet#1108; add ClusterDirectoryService…

9cc0d1a

… to singletonSystemTargetNames

sebastianburckhardt added a commit to sebastianburckhardt/orleans that referenced this pull request Dec 4, 2015

fix comments as discussed in dotnet#1108; add ClusterDirectoryService…

5a64c60

… to singletonSystemTargetNames

jthelin added the Status: work-in-progress label Jan 10, 2016

jdom reviewed Jan 13, 2016
View reviewed changes

sergeybykov reviewed Jan 28, 2016
View reviewed changes

sergeybykov reviewed Jan 30, 2016
View reviewed changes

GrainDirectory Refactoring and Multi-Cluster Registration (dotnet#1108)

1fa6036

sebastianburckhardt force-pushed the geo-graindirstruct branch from 09839f1 to 1fa6036 Compare February 1, 2016 17:31

jdom added a commit that referenced this pull request Feb 2, 2016

Merge pull request #1108 from sebastianburckhardt/geo-graindirstruct

243f811

GrainDirectory Refactoring and Multi-Cluster Registration

jdom merged commit 243f811 into dotnet:master Feb 2, 2016

sebastianburckhardt deleted the geo-graindirstruct branch February 2, 2016 16:16

jthelin removed the Status: work-in-progress label Feb 2, 2016

sergeybykov modified the milestone: Multi-Cluster Nov 3, 2016

github-actions bot locked and limited conversation to collaborators Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GrainDirectory Refactoring and Multi-Cluster Registration #1108

GrainDirectory Refactoring and Multi-Cluster Registration #1108

sebastianburckhardt commented Dec 3, 2015

dnfclas commented Dec 3, 2015

jdom Dec 3, 2015

sebastianburckhardt Dec 3, 2015

gabikliot Dec 4, 2015

gabikliot Dec 4, 2015

sebastianburckhardt Dec 4, 2015

sebastianburckhardt Dec 8, 2015

gabikliot commented Dec 22, 2015

jdom Jan 13, 2016

sebastianburckhardt Jan 14, 2016

sergeybykov Jan 28, 2016

sebastianburckhardt Jan 28, 2016

sergeybykov commented Jan 29, 2016

sebastianburckhardt commented Jan 29, 2016

sergeybykov commented Jan 29, 2016

sergeybykov Jan 30, 2016

sebastianburckhardt Feb 1, 2016

sergeybykov commented Jan 30, 2016

sebastianburckhardt commented Feb 1, 2016

jdom commented Feb 1, 2016

jdom commented Feb 2, 2016

sergeybykov commented Feb 2, 2016

gabikliot commented Feb 2, 2016

jthelin commented Feb 2, 2016

GrainDirectory Refactoring and Multi-Cluster Registration #1108

GrainDirectory Refactoring and Multi-Cluster Registration #1108

Conversation

sebastianburckhardt commented Dec 3, 2015

dnfclas commented Dec 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabikliot commented Dec 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeybykov commented Jan 29, 2016

sebastianburckhardt commented Jan 29, 2016

sergeybykov commented Jan 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergeybykov commented Jan 30, 2016

sebastianburckhardt commented Feb 1, 2016

jdom commented Feb 1, 2016

jdom commented Feb 2, 2016

sergeybykov commented Feb 2, 2016

gabikliot commented Feb 2, 2016

jthelin commented Feb 2, 2016