Add retry options #94
Conversation
satvu
requested review from
AnatoliB,
Francisco-Gamino,
YunchuWang,
alrod,
amamounelsayed,
ejizba,
gavin-aguiar,
kaibocai,
shreyas-gopalakrishna,
surgupta-msft and
vrdmr
as code owners
|
|
||
| // Retry policy which the worker sends the host when the worker indexes | ||
| // a function. | ||
| message RpcRetryOptions |
There was a problem hiding this comment.
Named RpcRetryOptions to avoid confusion with RetryOptions in the host. Precedent set by RpcFunctionMetadata and FunctionMetadata.
There was a problem hiding this comment.
So If a function definition is decorated with this information, will the retrycontext populated as part of invocation request will use this info instead of our existing RetryContext prop value (which is populated from host.json?) ?
There was a problem hiding this comment.
I wonder we should rename the prop/type to be more clear that this is something function authors may define on the function level?
There was a problem hiding this comment.
The invocation request is created here in GrpcWorkerChannel and the RetryContext field of the invocation request is populated through an extension method here that pulls from an ExecutionContext to get retry information.
I believe the step in ExecutionContext creation that populates retry information is here and pulls from FunctionMetadata, which uses info from RpcRetryOptions. Does that answer your question @kshyju?
There was a problem hiding this comment.
@kshyju While I think it's true that only trigger types that have function-level retry should be using this path, I'm not sure about giving this type a different name (like FunctionLevelRetryOptions) because it's still a representation of the webjobs/host RetryOptions. Also, if we use this type in other ways in the future aside from a reference in RpcFunctionMetadata, it may be more appropriate to have a general name matching with webjobs.
Happy to hear suggestions from others too- it's an easy change at this stage.
As a middle-ground solution I can also revise the comment for this type or add one to the corresponding RpcFunctionMetadata field to give a more detailed explanation of this type and its usage?
brettsam
left a comment
brettsam
left a comment
There was a problem hiding this comment.
lgtm -- but wait for confirmation from other language experts on that Duration usage
Related to host issue #9104. Related host PR #9265.
This update allows for retry policy information to be passed through the worker-indexing code path.