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

More options formatting, and related cleanup #3947

Merged
merged 2 commits into from Feb 2, 2018

Conversation

jason-bragg
Copy link
Contributor

Added formatters for grain directory options, grain placement options, schedualling options, thread pool options, type managment options, versioning options and silo options.
Fixed formatting of messaging options.
Fixed some options defaults.

this.options = options;
}

protected List<string> FormatStatisticsOptions()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naming: FormatMessagingOptions

Copy link
Contributor

@xiazen xiazen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for the naming comment. can merge after you fixed the naming

Added formatters for grain directory options, grain placement options, schedualling options, thread pool options, type managment options, versioning options and silo options.
Fixed formatting of messaging options.
Fixed some options defaults.
Cleaned up registeration order, it seems options are logged in order of registeration (by default).  Not an expected or necessary behavior.
@@ -25,4 +27,28 @@ public class SiloOptions
public bool FastKillOnCancelKeyPress { get; set; } = DEFAULT_FAST_KILL_ON_CANCEL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure when this was added, but I wouldn't put this in Orleans' specific SiloOptions. We didn't copy everything related to IHost but the IHostLifetime already has things such as these when you use the ConsoleHostLifetime way of hosting... Not suggesting you copy everything related to that (or maybe you do want to), but at least keep it isolated, since it will go away when using Microsoft.Extensions.Hosting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdom it should go away, but we're not using Microsoft.Extensions.Hosting yet. I see that we were both against having any kind of Ctrl+C handling in the first place: #3109

Hopefully we can remove it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to #3957

@xiazen xiazen merged commit 00bab73 into dotnet:master Feb 2, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 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

4 participants