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

. #3324

Closed
wants to merge 3 commits into from
Closed

. #3324

wants to merge 3 commits into from

Conversation

ericsciple
Copy link
Collaborator

No description provided.

@@ -62,7 +62,9 @@ public Task<AgentJobRequestMessage> GetJobMessageAsync(string id, CancellationTo
CheckConnection();
return RetryRequest<AgentJobRequestMessage>(
async () => await _runServiceHttpClient.GetJobMessageAsync(requestUri, id, VarUtil.OS, cancellationToken), cancellationToken,
shouldRetry: ex => ex is not TaskOrchestrationJobAlreadyAcquiredException);
shouldRetry: ex => ex is not TaskOrchestrationJobNotFoundException &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

keep, but maybe also keep a delay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't make this change until we also figure out how to keep a delay

@@ -1539,6 +1539,26 @@ private TaskOrchestrationJobAlreadyAcquiredException(SerializationInfo info, Str
}
}

[Serializable]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

keep

@@ -0,0 +1,17 @@
using System.Runtime.Serialization;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

keep

return true;
}
}
catch (Exception)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bubble this, so we can work it into the caller exception message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe log the first N characters

@@ -106,10 +106,11 @@ public void Dispose()
Uri requestUri,
HttpContent content = null,
IEnumerable<KeyValuePair<String, String>> queryParameters = null,
Boolean readErrorContent = false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

keep changes to this file

public string Error { get; protected set; }

/// <summary>
/// The raw of the HTTP response, for unsuccessful HTTP status codes
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

"The raw string content"

@@ -5,29 +5,41 @@ namespace Sdk.WebApi.WebApi
public class RawHttpClientResult
{
public bool IsSuccess { get; protected set; }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

keep changes to this file

var errorContent = default(string);
if (readErrorContent)
{
errorContent = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can this throw? if so, then can set the error content to the exception when trying to read

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if it throws because we hit a network error, then we want to let that bubble

@ericsciple ericsciple closed this Jun 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant