Support new API fields in ActivityHandle.describe - #1782
Conversation
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| @dataclass(eq=False, kw_only=True) |
There was a problem hiding this comment.
Why are you unfreezing the dataclasses?
There was a problem hiding this comment.
Mostly to allow swapping out data converter, but maybe that's wrong design. I can see the argument to keep these frozen, I'll re-add it.
| info=resp.info, | ||
| long_poll_token=resp.long_poll_token or None, | ||
| # Erase unrequested fields if server sent them anyway (can happen with old server) | ||
| if not input.include_input: |
There was a problem hiding this comment.
This seems a little sketchy
There was a problem hiding this comment.
Currently (or until recently), the server sends heartbeat details and last failure unconditionally. This piece of code ensures the user has consistent view of data even if the server doesn't respect the default/false values in the request. One alternative is to keep the data in the proto but persist the include_ args in the description class and filter these out in the getters. Another is to just allow the possibility that these fields may be set anyway even if not requested. Of the three, I think erasing it from the proto is the best approach.
There was a problem hiding this comment.
I would probably lean towards just leaving it. Seems hard for there to be a real failure mode where the user needs to not get it. Clearing it is just pretending we're being more efficient.
10cfb2a to
794894e
Compare
794894e to
7cfb7de
Compare
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| @dataclass(frozen=True, eq=False, kw_only=True) |
There was a problem hiding this comment.
Is there a good reason to add these where they weren't there before?
There was a problem hiding this comment.
ActivityExecutionDescription has dataConverter now so it can't have the automatic eq. Even for ActivityExecution, there's a possibility we add new fields in the future that will not work with eq. I also question the utility of having eq here. If a valid use case arises it can always be re-added later; removing it later won't be possible after stabilization.
Arguably, these should always have been kw_only as there are dozens of fields and the order is quite arbitrary.
Drops the four include_* opt-ins, the eager payload members on ActivityExecutionDescription, and the tests that exercise them, so that #1782 owns the describe surface outright. Operator commands, the ActivityOptionsKey/Update API and ActivityExecutionOptions are untouched. Heartbeat-detail observation is impossible until #1782 lands: the server withholds those payloads without the opt-in, so the three *_preserves_heartbeat tests are removed here and should return afterwards using 1782's await desc.heartbeat_details().
2 participants
What was changed
@dataclassconfiguration for ActivityExecution and ActivityExecutionDescriptionWhy?
Implements newly added API features and removes parts that were not meant to be exposed.
Checklist
Updated
tests.test_activity.TestDescribe