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

Moving retry logic for UpgradeApplication to corresponding helper fun… #7593

Merged
merged 6 commits into from
Jul 3, 2018

Conversation

kuvinodms
Copy link

…ction

@@ -240,7 +240,16 @@ function Get-ServiceFabricApplicationUpgradeAction
)

$global:operationId = $SF_Operations.GetApplicationUpgradeStatus
return Get-ServiceFabricApplicationUpgrade -ApplicationName $ApplicationName

Copy link
Contributor

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") `
Copy link
Contributor

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.

Copy link
Author

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
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

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

@kuvinodms kuvinodms requested a review from sachinma July 3, 2018 06:04
Copy link
Contributor

@bishal-pdMSFT bishal-pdMSFT left a 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.

@kuvinodms kuvinodms merged commit dcd941c into master Jul 3, 2018
kuvinodms pushed a commit that referenced this pull request Jul 17, 2018
#7593)

* Moving retry logic for UpgradeApplication to corresponding helper function

* Updated function name

* Added new operation

* Indentation

* Adding timeout exception
kuvinodms pushed a commit that referenced this pull request Jul 17, 2018
#7593)

* Moving retry logic for UpgradeApplication to corresponding helper function

* Updated function name

* Added new operation

* Indentation

* Adding timeout exception
kuvinodms pushed a commit that referenced this pull request Jul 17, 2018
* [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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants