Support for custom placement strategies and directors #2932
Conversation
…lacementRuntime. Make PlacementTarget and PlacementStrategy public. Replace PlacementTarget.GrainId with PlacementTarget.GrainIdentity. Make constructor of PlacementAttribute protected.
…internal IActivationSelector.
src/Orleans/Core/IGrainIdentity.cs
Outdated
|
|
||
| Guid GetPrimaryKey(out string keyExt); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: most of the codebase uses spaces for indentation
| var director = this.ResolveDirector(strategy); | ||
| if (director is IActivationSelector) | ||
| this.ResolveSelector(strategy); | ||
| } |
There was a problem hiding this comment.
what was wrong with this approach? it was a nice one to avoid issues if more built in strategies support selection in the future. The only thing that was missing was passing true to ResolveSelector, right?
Anyway, I think it's OK regardless, even if my preference is the previous approach.
There was a problem hiding this comment.
The problem was rather subtle. RandomPlacementDirector implements both IPlacementDirector and IActivationSelector. ActivationCountPlacementDirector extends RandomPlacementDirector, but it only really implements IPlacementDirector, though the check is IActivationSelector returns true for it. That was causing a problem.
I considered splitting RandomPlacementDirector into separate classes for placement director and activation selector, but thought it would cause even more code churn. Can be done as a separate change later.
|
|
||
| public PlacementTarget(IGrainIdentity grainIdentity, int interfaceId, ushort interfaceVersion) | ||
| { | ||
| GrainIdentity = grainIdentity; |
There was a problem hiding this comment.
be careful with tabs in several places
There was a problem hiding this comment.
Weird. Must be different setting on my desktop. I fixed them, but Git seems to be ignoring the changes.
| } | ||
|
|
||
|
|
||
| public enum CustomePlacementScenario |
There was a problem hiding this comment.
very nit: no 'e' after "Custom"
| @@ -39,15 +33,15 @@ internal interface IPlacementContext | |||
| void GetGrainTypeInfo(int typeCode, out string grainClass, out PlacementStrategy placement, out MultiClusterRegistrationStrategy strategy, string genericArguments = null); | |||
| } | |||
|
|
|||
| internal static class PlacementContextExtensions | |||
| internal static class PlacementContextExtensions | |||
There was a problem hiding this comment.
Minor: rename to PlacementRunTimeExtensions?
| this.selectors[strategyType] = result; | ||
| } | ||
|
|
||
| return result ?? defaultActivationSelector; |
There was a problem hiding this comment.
Not very familiar with the code. But basic question: In line number 54 result is assigned to this.selectors[strategyType]. In line 57 if result is null --- defaultActivationSelector is returned. Should we update the this.selectors[strategyType] to default if result is null?
There was a problem hiding this comment.
Not very familiar with the code.
Me neither. :-) result can be null if TryGetValue returns false. That's the case when we fall back to the default selector.
|
There's several other tabs that were introduced. Do you want me to fix them? I have push access to your repo |
|
@jdom By all means. Thank you. |
jdom
force-pushed
the
public-placement-3
branch
from
428cdae
to
9c2b540
Compare
|
Done, I forced pushed to the |
|
Perfect. |
IPlacementDirectortoday defines two methods:OnAddActivationandOnSelectActivation. The latter is used only for a couple of special cases, and custom placement policies only need to be concerned with the former.OnSelectActivationdepends on a number of internal types, whileOnAddActivationdoes not.I split the two operations between two interfaces: public
IPlacementDirectorfor custom (and built-in) placement directors to implement and internalIActivationSelectorfor those special internal (for now) cases. A few more types needed to be made public, but those were easy.This should enable #2764 and #96.
Placement strategies for now have to be serializable. We can look at removing this requirement later.