-
Notifications
You must be signed in to change notification settings - Fork 1
Add request deadline to OperationContext subclasses #30
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
Conversation
|
Because this deadline is related to requests and not the operation, should we consider putting this in the cancellation object I added in #31? Might help to keep the options related to task cancellation together. We can also get around the ordering issue and keep the default by making it a keyword only field, which seems fairly reasonable to me. Though if we add it to the task cancellation object we'd also avoid it. request_deadline: Optional[datetime] = field(default=None, kw_only=True)
"""
Get the deadline for the operation handler method. Note that this is the time by which the
current _request_ should complete, not the _operation_'s deadline.
""" |
|
I moved the request deadline to |
src/nexusrpc/handler/_common.py
Outdated
| """ | ||
|
|
||
| headers: Mapping[str, str] | ||
| headers: Mapping[str, str] = field(default_factory=dict) |
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.
Not sure this field needs a default, it never had one before
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 guess my concern was more that the comment says it is optional, but the field's type isn't. So the field's default factory was to consolidate those two contradicting facts, but I can alternatively change the doc or make the field's type Optional. No strong opinion on those options.
Also, tbh, I only have little prior knowledge of Python dataclasses, so it is totally possible that I'm misunderstanding something here.
With that in mind, do you maintain I should remove the default?
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 think we should change the doc to remove the word optional and leave the default value off. For users that are implementing an operation handler, they will still always have a Mapping to inspect. For users implementing a worker (such as the Temporal worker), they'll be required to provide a Mapping which will then encourage them to either consciously default to an empty dict or think about how to handle headers in a more robust way.
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 guess my concern was more that the comment says it is optional
The comment says that it's optional for the caller to have ever set these per Nexus spec, it has nothing to do with whether the field type is optional for the creator of this object (which is us). There is a difference between what is optional for a caller in the Nexus spec and what is optional for us when instantiating a class.
src/nexusrpc/handler/_common.py
Outdated
| Task cancellation information indicating that a running task should be interrupted. This is distinct from operation cancellation. | ||
| """ | ||
|
|
||
| request_deadline: Optional[datetime] = field(default=None) |
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.
| request_deadline: Optional[datetime] = field(default=None) | |
| request_deadline: Optional[datetime] = None |
Don't need the field part, None is immutable/acceptable here
src/nexusrpc/handler/_common.py
Outdated
| """ | ||
|
|
||
| callback_url: Optional[str] = None | ||
| callback_url: Optional[str] = field(default=None) |
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.
| callback_url: Optional[str] = field(default=None) | |
| callback_url: Optional[str] = None |
Same as other place
Description
request_deadlinefield toOperationContextandCancelOperationContext. Resolves Expose Request Deadline on Operation Context #29.OperationContextandStartOperationContextaskwonly.