Multi-Cluster Network #1109
Multi-Cluster Network #1109
Conversation
|
@sebastianburckhardt, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| // deterministic function for designating the silos that should act as gateways | ||
| private List<SiloAddress> DetermineMultiClusterGateways() | ||
| { | ||
| Debug.Assert(!string.IsNullOrEmpty(GlobalServiceId)); // call only if this is a multi cluster |
There was a problem hiding this comment.
No Debug.Assert in the code.
You can throw ArgumentExc or another reasonable exc.
There was a problem hiding this comment.
Ok, done.
sebastianburckhardt
force-pushed
the
geo-multiclusternetwork
branch
from
12b5f17
to
02297ac
Compare
| { | ||
| ChannelType = (GlobalConfiguration.GossipChannelType) | ||
| Enum.Parse(typeof(GlobalConfiguration.GossipChannelType), channelspec.GetAttribute("Type")), | ||
| ConnectionString = channelspec.GetAttribute("ConnectionString") |
There was a problem hiding this comment.
not sure this is the right place for a change, but do you think that if ConnectionString is null we should fall back to using GlobalConfiguration.DataConnectionString or we should try to keep them explicitly separated given that one's purpose is for intra-cluster storage and the other global communication?
There was a problem hiding this comment.
I think it is better to fail if ConnectionString is null, and force the user to specify it, than to use a default that seems to work superficially but does not really do the right thing
| [DeploymentItem("OrleansAzureUtils.dll")] | ||
| [DeploymentItem("TestGrainInterfaces.dll")] | ||
| [DeploymentItem("TestGrains.dll")] | ||
| [DeploymentItem("ClientConfigurationForTesting.xml")] |
There was a problem hiding this comment.
Need to move to programmatic config instead of the xml files.
There was a problem hiding this comment.
Tests were only partially migrated. That's done now, I have merged latest master.
|
I think I'm done with my comments. So if we resolve all outstanding questions/suggestions, this should be ready for a rebase/squash/merge. |
|
Looks nearly done to me. Only a few outstanding comments left. The most important one I think is https://github.com/dotnet/orleans/pull/1109/files#diff-82d86f0f90ad13e9e64a997b86a9f8ffR499. When the remaining comments are addressed, we should be able to rebase and merge. |
|
I have addressed all outstanding issues, I think. One of the changes was significant: Instead of using chained tasks for gossiping, I now use a batchworker (a pattern that I have used several other times, it will also be used in queued grain code). |
|
I will rebase and squash into a single commit by noon today if there are no objections. @jdom, if you prefer to preserve some history and sculpt several commits like you did last time, let me know. |
| } | ||
|
|
||
| // task for the current work cycle, or null if idle | ||
| private Task currentworkcycle; |
There was a problem hiding this comment.
Camel case for private fields is the standard.
|
I think I have addressed all the comments. Ready to squash and rebase? |
|
Need to rebase on top of master. Otherwise, looks good to me. Squashing can be done by GitHub, now that @jdom showed me where that button is hidden. Unless, you think it makes sense to group the changes into a small set of cohesive commits. |
|
@jdom, it's up to you if you want to sculpt commits like you did last time or not. I am not sure it's worth it (there is a ton of history hidden away anyway - that history still exists in our other repositories if we ever need it) |
|
As you wish, I guess at this point the easiest would be to squash everything into 1 commit. Are you confident doing that, making sure you don't revert other people's changes while merging first? |
sebastianburckhardt
force-pushed
the
geo-multiclusternetwork
branch
from
73dae59
to
50c5aad
Compare
|
I think this is ready. |
This branch contains the new functionality for supporting multiple clusters (Multi-Cluster Network, Azure-Table-Based Gossip Channels, and Multi-Cluster Unit Tests). It is one of the pieces of the Geo-Distribution Project #948.