App Service Manage task review comments #6143
Conversation
| await appService.monitorAppState("stopped"); | ||
| await appService.pingApplication(pingApplicationCount); | ||
| await azureAppServiceUtils.monitorApplicationState("stopped"); | ||
| await azureAppServiceUtils.pingApplication(); |
There was a problem hiding this comment.
We dont fail here for non 200 codes as our primary concern is to hit the app once and avoid cold-start (delay in first request to the app).
There was a problem hiding this comment.
Just curious, why do we need it in stop?
There was a problem hiding this comment.
Just an additional step. Even when stop action is invoked, it does take some time to take effect and result in cold start. First request made during the post action may result in 200 return code leading to wrong impression of the state. So, we explicitly ping here. (Also, it helps to warm-up the slot,if used before deploy task)
There was a problem hiding this comment.
After stop the site is stopped - so there shouldn't be any response. And warm-up doesn't make sense in stopped web-site.
There was a problem hiding this comment.
removed warm-up logic
| break; | ||
| } | ||
| case "Restart Azure App Service": { | ||
| var appService: AzureAppService = new AzureAppService(azureEndpoint, resourceGroupName, webAppName, slotName); | ||
| await appService.restart(); | ||
| break; |
There was a problem hiding this comment.
Why don't we wait for the state or ping in restart
There was a problem hiding this comment.
Restart has less significance compared to start/stop. We should be good here.
There was a problem hiding this comment.
Warm-up should be required after restart
There was a problem hiding this comment.
adding warm-up logic after restart,
| @@ -213,6 +215,7 @@ export interface AzureEndpoint { | |||
| portalEndpoint?: string; | |||
| AzureKeyVaultDnsSuffix?: string; | |||
| AzureKeyVaultServiceEndpointResourceId?: string; | |||
| applicationTokenCredentials: ApplicationTokenCredentials; | |||
There was a problem hiding this comment.
Should this be a method here instead given this is derived value? like getArmTokenCredentials
There was a problem hiding this comment.
Planned to have attributes here instead of methods.
Also, it tends to honour as an attribute for AzureEndpoint.
| console.log(tl.loc('WarmingUpSlots')); | ||
| await appServiceSourceSlot.pingApplication(1); | ||
| await appServiceTargetSlot.pingApplication(1); | ||
| await appServiceSourceSlotUtils.pingApplication(); |
There was a problem hiding this comment.
consider doing ping in parallel and then awaiting both for better perf
There was a problem hiding this comment.
sure. will do the same.
vincent1173
added
the
Area: AzureAppService
No description provided.