-
Notifications
You must be signed in to change notification settings - Fork 914
Enhancing the delay in the listener when there is exception in the connection #5448
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
base: master
Are you sure you want to change the base?
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
rajmishra1997
left a comment
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.
Suggestion: For higher number of error response from server, can we add log/warning to display continuous error response received
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
ad39d51 to
16a1ba3
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Please explore how we can simplify the currently spreaded logic of retries. If possible, we can have common function for calculating retry delay which we are having and multiple places in message listener right now. |
|
we are not enabling FF to RM here in the PR , could you please check if do we need. https://dev.azure.com/mseng/AzureDevOps/_git/AzureDevOps/pullrequest/891094?_a=files |
… calls and also added a FF for the changes
…st cases accordingly
2ab1629 to
6be937b
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
b7977f4 to
05c42b5
Compare
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Context
Adding a delay in the Agent listener code before retrying request when there is a retriable exception thrown from the server side.
Related work-item: AB#2338439
Description
When the server is unavailable (not an authentication error), the server returns a different exception (e.g., VssServiceResponseException with status code 404). The agent continuously retries the request indefinitely. Each retry invokes the OAuth token provisioning mechanism on the server (the server checks whether there is any cached token or not else it creates one). This behavior significantly increases load on an already unavailable server.
Hence we have increased the Backoff delay in the agent code before retrying the requests. We have implemented an exponential backoff delay which is capped at 5 minutes and all the changes are behind a Feature flag so that the execution can be controlled.
Risk Assessment (Low / Medium / High)
Low
Unit Tests Added or Updated (Yes / No)
Yes added L0 test cases for the delay behaviour in 3 methods CreateSessionAsync, GetNextMessageAsync and KeepAlive based on the state of the feature flag added.
Additional Testing Performed
Manually tested connecting the agent with devfabric and then stopping the tfs devfabric web service. The agent delayed the request based on the continuous error count.
For the main listener loop:
For the Keep Alive Call:
For Create Session Call:
[when there is session conflict]
Change Behind Feature Flag (Yes / No)
No
Tech Design / Approach
This is done to reduce the load on the server where the agent continuously make requests to the server.
Documentation Changes Required (Yes/No)
No
Logging Added/Updated (Yes/No)
NA
Telemetry Added/Updated (Yes/No)
NA
Rollback Scenario and Process (Yes/No)
NA
Dependency Impact Assessed and Regression Tested (Yes/No)
NA