[Storage][Core] Wait for environment to become eventually consistent. #20574
Conversation
ghost
added
sdk/core/Azure.Core.TestFramework/src/RecordedTestBaseOfTEnvironment.cs
Outdated
Show resolved
Hide resolved
kasobol-msft
requested review from
amishra-dev,
KrzysztofCwalina and
tg-msft
as code owners
|
/azp run net - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| /// Throw if you want to fail the run fast. | ||
| /// </summary> | ||
| /// <returns>Whether environment is ready to use.</returns> | ||
| public virtual Task<bool> IsEnvironmentReady() |
| /// </summary> | ||
| /// <returns>A task.</returns> | ||
| public async Task WaitForEnvironment() | ||
| { |
There was a problem hiding this comment.
We might have a problem if multiple classes call this method in parallel. Consider having an IDictionary<Type, Task> where we initialize the task immediately and then await on it.
There was a problem hiding this comment.
It would also have a nice side-effect of caching the exception.
There was a problem hiding this comment.
yup and it got rid of the enum.
There was a problem hiding this comment.
Why do we need per-class invocations of WaitForEnvironmentInternal? The logic can't vary per test class, right?
There was a problem hiding this comment.
We call into virtual method where service team provides sampling scenario. I don't think we can do this from static context.
There was a problem hiding this comment.
Nevermind, this is per Environment type.
|
/azp run net - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -23,5 +27,11 @@ public override void StartTestRecording() | |||
| } | |||
|
|
|||
| public TEnvironment TestEnvironment { get; } | |||
|
|
|||
| [OneTimeSetUp] | |||
| public async Task WaitForEnvironment() | |||
There was a problem hiding this comment.
Should we make this ValueTask since it will usually complete synchronously?
There was a problem hiding this comment.
If somebody decides they need "sampling" they'll most likely end up calling API like this:
https://github.com/kasobol-msft/azure-sdk-for-net/blob/dd244f41ee9c999dc09cb42cbd72930f1320770d/sdk/storage/Azure.Storage.Blobs/tests/BlobTestEnvironment.cs#L12-L30
So, this task here would chain to async api call worst case.
Could you elaborate more how would you use ValueTask in this logic?
There was a problem hiding this comment.
Well, if the method isn't overriden then it would complete synchronously. But I suppose since we are returning Task<bool> the runtime is able to use cached instances for these so it probably doesn't provide much benefit. Also, we'd have to call AsTask anyway when putting them into the dictionary, so I think using Task makes sense.
https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/#task
There was a problem hiding this comment.
Good read. How does it look now?
One place I had to retain task is the cache as we shouldn't be await on ValueTasks multiple times (per that article).
There was a problem hiding this comment.
Yep, that is what I was thinking when I mentioned needing to call AsTask, but I think how you have it is even better.
There was a problem hiding this comment.
Being that we still have to allocate a task, I don't think there is any practical difference between using ValueTask<T> and Task<T> here, but ValueTask still seems to convey the intent of the API better. /cc @pakrym to keep me honest here
There was a problem hiding this comment.
I would say task vs valuetask doesn't matter in this situation at all. We have so many way larger inefficiencies in the test framework.
But ValueTask<T> and Task<T> difference matters for sync completion mostly. ValueTask<T> won't allocate when completed synchronously. But Task<bool> return value are cached(by the runtime) so it won't allocate as well
| /// Waits until environment becomes ready to use. See <see cref="IsEnvironmentReady"/> to define sampling scenario. | ||
| /// </summary> | ||
| /// <returns>A task.</returns> | ||
| public async Task WaitForEnvironment() |
There was a problem hiding this comment.
Consider suffixing with Async.
| /// Throw if you want to fail the run fast. | ||
| /// </summary> | ||
| /// <returns>Whether environment is ready to use.</returns> | ||
| protected virtual Task<bool> IsEnvironmentReady() |
There was a problem hiding this comment.
Consider suffixing with Async.
|
This is really cool. Would you like to add a blurb to the Readme - https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/core/Azure.Core.TestFramework/README.md |
|
/azp run net - storage - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Adding archive/unarchive to Microsoft.Capacity 2022-03-01 (Azure#20574) * Adding archive/unarchive to Microsoft.Capacity 2022-03-01 * fix * format
This is another attempt to solve #17384 and Azure/azure-sdk-tools#1567 .
Based on discussion in #20559 .
The idea is that:
TestEnvironmentexposes a overridable method that represents service provided sampling scenarioRecordedTestBase<TEnvironment>shall keep executing the sampling scenario until it signals it's ready (or fail fast if the error is severe)Out of scope:
StorageTestBasetoStorageTestEnvironment. However some work towards it is accomplished here.RecordedTestBase<TEnvironment>is simplified now. I think we should make it fancy when there's a need.