Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify UseDevelopmentClustering and UseLocalhostClustering #4438

Merged
merged 5 commits into from Apr 24, 2018

Conversation

ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Apr 10, 2018

Fixes #4421

@ReubenBond
Copy link
Member Author

ReubenBond commented Apr 10, 2018

I removed ClusterId & ServiceId parameters from the extension methods.

That means that there is a single way to specify ClusterId/ServiceId (ignoring legacy config).
The values will still default to "dev" when using UseLocalhostClustering, but only if no value was specified (and otherwise the startup would throw, once that validation is added).

In my opinion, this is cleaner. It means that users will have to manually specify the value when using UseDevelopmentClustering, but this seems alright to me.

UseDevelopmentClustering on the silo is paired with UseStaticClustering on the client. This is because there are other cases where the client might access the cluster using a well-known set of gateways. A static gateway list is fine for production use on the client. On the other hand, grain based membership is not fault tolerant and so it's usually not considered suitable for production.
UseLocalhostClustering on the silo is paired with UseLocalhostClustering on the client.

@ReubenBond
Copy link
Member Author

@benjaminpetit we discussed this for about 40 mins the other day. I thought we had decided to add clusterId + serviceId parameters to UseLocalhostClustering only, however when I went to implement it I found that it didn't feel so good. I think we should take this as-is and instead add a helper for configuring those two in a separate PR. wdyt?

@benjaminpetit
Copy link
Member

@ReubenBond let's discuss this live when you are available

@ReubenBond
Copy link
Member Author

Ok, how does this look, @benjaminpetit?

@sergeybykov sergeybykov added this to the 2.0.3 milestone Apr 24, 2018
@benjaminpetit benjaminpetit merged commit 08f3d0b into dotnet:master Apr 24, 2018
sergeybykov pushed a commit to sergeybykov/orleans that referenced this pull request May 5, 2018
)

* Try to clarify meaning of UseDevelopmentClustering and UseLocalhostClustering, plus include ServiceId in calls

* Remove ClusterId/ServiceId from UseDevelopmentClustering/UseLocalhostClustering extensions

* Only configure default ClusterId/ServiceId in UseLocalhostClustering

* Add ServiceId & ClusterId to UseLocalhostClustering extensions

* fix build
ReubenBond added a commit that referenced this pull request May 9, 2018
* Try to clarify meaning of UseDevelopmentClustering and UseLocalhostClustering, plus include ServiceId in calls

* Remove ClusterId/ServiceId from UseDevelopmentClustering/UseLocalhostClustering extensions

* Only configure default ClusterId/ServiceId in UseLocalhostClustering

* Add ServiceId & ClusterId to UseLocalhostClustering extensions

* fix build
@ReubenBond ReubenBond deleted the clarify-clustering branch February 6, 2019 18:57
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants