-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Moving retry logic for UpgradeApplication to corresponding helper fun… #7593
Conversation
@@ -240,7 +240,16 @@ function Get-ServiceFabricApplicationUpgradeAction | |||
) | |||
|
|||
$global:operationId = $SF_Operations.GetApplicationUpgradeStatus | |||
return Get-ServiceFabricApplicationUpgrade -ApplicationName $ApplicationName | |||
|
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.
This should be a separate function Wait-ServiceFabricApplicationUpgrade. get should just get the status and not wait for it to finish
-ResultRetryEvaluator $upgradeRetryEvaluator ` | ||
-MaxTries 2147483647 ` | ||
-RetryIntervalInSeconds 3 ` | ||
-RetryableExceptions @("System.Fabric.FabricTransientException") ` |
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.
Should we add TimeoutException here as well.
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.
Adding timeout exception is not a harm. Adding!
@@ -243,6 +243,26 @@ function Get-ServiceFabricApplicationUpgradeAction | |||
return Get-ServiceFabricApplicationUpgrade -ApplicationName $ApplicationName | |||
} | |||
|
|||
function Wait-ServiceFabricApplicationUpgradeAction |
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.
Please think about adding L0 test for this function.
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.
Its a minor work but I don't want to spend any more time over this. Either I'll add it later or you take it up.
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.
You own this PR and hence you should decide whether to add test or not.
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.
Please approve if there are not any further concerns.
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.
If you are not adding L0, open a user story/task to track that.
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.
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.
Please take care of adding L0s in future PRs.
#7593) * Moving retry logic for UpgradeApplication to corresponding helper function * Updated function name * Added new operation * Indentation * Adding timeout exception
#7593) * Moving retry logic for UpgradeApplication to corresponding helper function * Updated function name * Added new operation * Indentation * Adding timeout exception
* [ServiceFabric] Re-tries for Unregister, connect, create and remove commands (#7588) * Re-tries for copy and register commands * PR comments * PR comments and few suggestion as per Oana * connect, unregister, create, remove re-tries * L0 for unregister retry * L0 for create and remove application * Utilities changes which were not picked in merge * PR comments * Moving retry logic for UpgradeApplication to corresponding helper fun… (#7593) * Moving retry logic for UpgradeApplication to corresponding helper function * Updated function name * Added new operation * Indentation * Adding timeout exception * [Service fabric] Retry for remaining commands (#7607) * Retry for remaining commands * PR comment * Adding logs for upgrade service fabric application. (#7217) * Adding logs for upgrade service fabric application. * resolving comments * Forming upgrade errors message. * Formatting unhealthy evaluation and upgrade status. * fixed merge changes * Formatting UnhealthyEvaluation and DomainUpgradeStatus while printing only conditionally. * Moved waiting for upgrade to complete and printing logic to function in Utilities file. * Keeping other exception as well. * Implemented async operations (#7631) * Implemented Async operations * Added null checks * Modified code as per review comments * typo * Revert "typo" This reverts commit f04d14e. * Revert "Modified code as per review comments" This reverts commit e24bc53. * Modified code as per review comments * Modified code as per review comments * Addressed review comments * Added retry logic to Wait-ServiceFabricApplicationTypeRegistrationStatus * Added comments * Addressed review comments * typo * Added comments * typo * Addressed review comments * Updated test cases * Modified code as per review comments * Users/hiyada/s ftask log bug fixes (#7722) * Corrected typo. * Corrected retry message null check * Addressed bugs raised as a part of bugbash (#7739) * Added powershell helper module * Replacing Trace-VstsEnteringInvocation with print statement * Removing Trace-VstsLeavingInvocation $MyInvocation * Addressed review comments * Updated patch version (#7748)
…ction