philips-software / powershell-cf-api

PowerShell module can deploy and teardown CloudFoundry spaces and services via a json definition file
MIT License
5 stars 6 forks source link

Discussion: `Invoke-Retry` as part of HTTP requests #33

Open lipkau opened 4 years ago

lipkau commented 4 years ago

I would like to start a discussion about having a retry logic as part of the module.

I vote for removing this with the following arguments:

  1. assuming how the user will use the module the retry functionality assumes that the commands should work. if a command is used in a way that an error is expected, the response will be very slow because of the retries. eg:
# iterate over all users and grant (those who have a cf user) SpaceDeveloper permissions
# this will fail with 404 for every user that does not exist on cf
Get-ADUser * | Foreach { Set-SpaceRole -Space (Get-Space "foo") -Username $_ -Role "developers" }
  1. complexity of output object if I were running a command with retry functionality, I would expect the response to contain the number of retries needed to succeed and the output when the command succeed. and in case it didn't, an object containing all of the exceptions/errors were it failed.

My suggestions:

  1. remove Invoke-Retry from the module and users who want such a functionality, need to implement it by themselves
  2. have Invoke-Retry as a help function as part of the module
marklindell commented 4 years ago

Lol, funny you just moved Invoke-Retry to private. Eh, that's why it's call "software"

I have thought about this before and would generally agree. My initial objective was to have a successful space creation from the aggregate command Publish-Space as 1) it is long running and 2) I am using this in a production system. I saw several HTTP hiccups that cause the space creation to fail and wanted to have retries for it to work and not have to start over. I admit that Invoke-Retry everywhere added under stress of trying to get another release out. ;-)

I was thinking the retry logic should be part of the wrapper function that would need to be created to handle Invoke-WebRequest/Invoke-RestMethod. By default it should limit retries to 500x status or transport errors.

What would be the harm in having that part of every Invoke-WebRequest? When would you not want to retry on 500 or transport error?

I really think these powershell API wrapper projects could benefit from a re-usable Invoke-WebRequest proxy. What do you think?

marklindell commented 4 years ago

Perhaps this could serve as a starting point: https://github.com/nicholasdille/PowerShell-WebRequest

marklindell commented 4 years ago

I just realized that both Invoke-WebRequest and Invoke-RestMethod have support for retries:

-MaximumRetryCount
Specifies how many times PowerShell retries a connection when a failure code between 400 and 599, inclusive or 304 is received. Also see RetryIntervalSec parameter for specifying number of retries.

-RetryIntervalSec
Specifies the interval between retries for the connection when a failure code between 400 and 599, inclusive or 304 is received. Also see MaximumRetryCount parameter for specifying number of retries.

Perhaps these two params are added to all cmdlets that call Invoke-WebRequest as a pass through. I still think the wrapper/proxy would be a good idea.

marklindell commented 4 years ago

Works great:

PS> $VerbosePrefrence="Continue"
PS> Invoke-WebRequest -Uri https://run.mocky.io/v3/31855807-b8a4-46c1-8e59-c735f00737d0 -MaximumRetryCount 2 -RetryIntervalSec 2
VERBOSE: GET https://run.mocky.io/v3/31855807-b8a4-46c1-8e59-c735f00737d0 with 0-byte payload
VERBOSE: Retrying after interval of 2 seconds. Status code for previous attempt: InternalServerError
VERBOSE: Retrying after interval of 2 seconds. Status code for previous attempt: InternalServerError
VERBOSE: received 0-byte response of content type application/json
Invoke-WebRequest: Response status code does not indicate success: 500 (Internal Server Error).