-
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
Testing Multiple Clusters #1128
Testing Multiple Clusters #1128
Conversation
string msg = string.Format("Using Azure local storage emulator = {0}", usingLocalWAS); | ||
Console.WriteLine(msg); | ||
Trace.WriteLine(msg); | ||
return usingLocalWAS; |
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.
are these (minor) side effects valuable?
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.
I think @jthelin added this, maybe he knows more about it.
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.
I'd remove it then. There is some code below that uses this. You can expand it to log this info if you want right there
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. My guess is he added these side effects in order to debug the testing environment. I am quite sure we can comment them out.
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.
I think it was something to do with diagnosing problems with the Azure storage configs on the old TFS build servers, but not sure how important it is to have this now.
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.
I have commented out the tracing statements.
This looks ready to merge now, and is fairly orthogonal functionality to current code anyway. Any outstanding concerns from anyone? @sebastianburckhardt - please squash to single commit for a simpler merge tree. |
@gabikliot any concerns with merging this (after the rebase cleanup)? Pinging you since you were the one who suggested having this as a separate PR to be able to review more thoroughly. |
4dd408c
to
c50297f
Compare
I have squashed the commits (and updated the dependent branches). |
if (host != null) | ||
host.AdjustForTest(config); | ||
|
||
if (options.ConfigurationCustomizer != 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.
why do we need the 2 ways to Adjust/Customize Configuration For Test? The host.AdjustForTest is not enough?
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.
host.AdjustForTest can only be used by subclassing TestingSiloHost so it doesn't work with TestingClusterHost. Passing configuration customization in the TestingSiloOptions is more flexible since individual tests can easily specify it, not just each subclass of TestingSiloHost.
Looks good. Please rebase to solve conflicts. |
Conflicts: src/OrleansTestingHost/TestingSiloHost.cs src/OrleansTestingHost/TestingSiloOptions.cs src/TesterInternal/TesterInternal.csproj
Done. |
…ting Testing Multiple Clusters
Contains a subset of PR #1109 with all the multi-cluster testing functionality, but none of the multi-cluster network implementation.