Move Kubernetes 410 exception handling outside generator #15755
Conversation
|
Hoping to get a check on whether this change is sensible and actually makes a difference. The original test still passes since the watch is being restarted, but I'm wondering if there's a better way to test this that proves moving the handling up a layer matters, and when the exception is raised by the stream has a real impact. |
kevingrismore
requested review from
cicdw,
desertaxle and
zzstoatzz
as code owners
There was a problem hiding this comment.
thanks @kevingrismore
is there some sort of integration test we can use to sanity check things here? perhaps a new CI job that could run when paths in src/prefect_kubernetes/** change?
we might be able to use this as inspiration. not saying it needs to happen here, just saying in general it might be nice to have for these cases where mocks come up a bit short
|
This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment. |
|
The updated test passes on both the original and changed location of the try-except, so I think it's possible this always worked as intended starting with the async version of the worker. This would just be a more accurate test and some code simplification, then. |
zzstoatzz
added
integrations
|
This pull request is stale because it has been open 14 days with no activity. To keep this pull request open remove stale label or comment. |
|
This pull request was closed because it has been stale for 14 days with no activity. If this pull request is important or you have more to add feel free to re-open it. |
The previous implementation tried to catch 410 exceptions inside the
_job_eventsgenerator, but continued reports of 410s indicate that it's likely that strategy would only detect a 410 if raised on the initial call tostream. This moves handling and retrying the watch to the caller.Checklist
<link to issue>"mint.json.