-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Remove confusing parameter from AzureSilo.Start #2109
Remove confusing parameter from AzureSilo.Start #2109
Conversation
@@ -174,6 +169,8 @@ public bool Start(ClusterConfiguration config, string deploymentId = null, strin | |||
|
|||
// Bootstrap this Orleans silo instance | |||
|
|||
var deploymentId = host.Config.Globals.DeploymentId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you can keep the method signature as is, and here do:
deploymentId = deploymentId ?? host.Config.Globals.DeploymentId ?? serviceRuntimeWrapper.DeploymentId;
This way the change will not be breaking, just doing fallbacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still confusing for the caller, I don't think it is bad to have a breaking change in this case. I think we should allow to pass a config or a deploymentId that will override the value from the config, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I also find this very confusing, and think it stems from when Orleans was XML config based only. I think that even today there are still deployments with a static XML configuration, but they want to just override the deploymentId in code. Granted, instead of just calling Start, they could go through the process of manually loading the XML config, then updating the deploymentId, and then calling start.
But I'd be wary about making this breaking change, when we are already thinking that several breaking changes to config/startup are coming in our Orleans 2.0 release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, looks like my concern is addressed in the latest rebase
d411f9b
to
07ae390
Compare
07ae390
to
fa7a810
Compare
// Program ident | ||
Trace.TraceInformation("Starting {0} v{1}", this.GetType().FullName, RuntimeVersion.Current); | ||
|
||
// Check if deployment id was specified | ||
if (deploymentId == null) | ||
deploymentId = serviceRuntimeWrapper.DeploymentId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this mean that if the user has an XML configuration, he must explicitly use serviceRuntimeWrapper directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't rely on serviceRuntimeWrapper here. If the user has an XML configuration and want to override the deploymentId, he should use Start(string deploymentId = null, string connectionString = null)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but does he have to manually pass in what was previously the default (the hosted service deployment id)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
The most common use case is that you use Azure's Hosted Service DeploymentId. Overriding is a nice to have. |
Yes, it is a breaking change.
But currently if you do something like this:
The Start method will partially ignore the DeploymentId that is in the configuration which is confusing.