Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement CreateHttpManagementPayload API in Durable Worker Extension #2929
base: dev
Are you sure you want to change the base?
Implement CreateHttpManagementPayload API in Durable Worker Extension #2929
Changes from all commits
0255475
48df767
376fb69
221138f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Quick nit - I noticed this returns
object
which is very general. In the WebJobs extension, we used to returnHttpManagementPayload
as the type, see here:azure-functions-durable-extension/src/WebJobs.Extensions.DurableTask/ContextImplementations/DurableClient.cs
Line 152 in 8a5db71
Should we not also introduced a worker extension version of this type, so that we can return a more precise return type?
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.
cc/ @cgillum, to confirm. Curious if it makes sense to return
object
for this particular API type, not sure if there's something I'm missingThere 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.
Oh, that's a good point. It made sense to use
object
and anonymous types in the case where we were serializing the object into the HTTP response. However, for the purposes of this new, more general purpose API, it's better to create a newHttpManagementPayload
class so that users of this method have a strongly-typed object they can work with. Thanks @davidmrdavid for bringing this to my attention.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.
Instead of catching and re-throwing the InvalidOperationException. Why don't we just throw a single exception inside of
SetHeadersAndGetPayload
and let that exception propagate out of this method? We may need to move the "Failed to create HTTP management payload" message to "SetHeadersAndGetPayload", but that seems 'ok' to me.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.
this is nitpicky, but I feel this is a bit difficult to read due the combination of ternary expressions the
<condition> ? <true-case> : <false-case>
syntax with the nullable syntax<value> ?? <code-to-execute-if-value-is-null>
.I just feel that there's a lot happening in very little space :) . I would using either the ternary expression, or the nullable syntax, but not both at the same time. The other can be expressed as a simple if-statement.
Not a blocking concern, just a stylistic nit. Feel free to dismiss.
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.
Haha I just used this so that I could save more space. I will change this to if-statement to make it more clear. Thanks!
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.
I'm not sure I understand what it means to be "from request". Can you please clarify?
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.
There are two ways to obtain the BaseUrl: (1) from HttpRequestData, which refers to the request in this context. And this usually happen when cx already have a Http bindings, or (2) from the Functions host, if HttpRequestData is not provided, as this information is passed through the DurableClient bindings.
Let me try to update this comment to be more clear.
Eg, calling CreateHttpManagementPayload with HttpRequestData like this:
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.
why is
response
possiblynull
now?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.
SetHeadersAndGetPayload
is also invoked by the existing method CreateCheckStatusResponse, which provides an HTTP response toSetHeadersAndGetPayload
. In the new API,CreateHttpManagementPayload
, there won’t be an HTTP response parameter. Therefore, whenSetHeadersAndGetPayload
is called byCreateHttpManagementPayload
, the response will be null; however, if it’s called byCreateCheckStatusResponse
, the response will not be null.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.
Is it allowed for this string to ever be null? If not, I'd recommend removing the
?
from thestring
type.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.
If the
DurableTaskClient
isn’t an instance ofFunctionsDurableTaskClient
, then HttpBaseUrl could be null, as it’s only an attribute ofFunctionsDurableTaskClient
. I’m not entirely sure in what scenario the DurableTaskClient wouldn’t be a FunctionsDurableTaskClient—perhaps it could happen if bindings aren’t being used. That’s why I added an exception for cases where the base URL creation fails.