fix(ingest/mode): Handle 204 response and invalid json #12156
Conversation
github-actions
bot
added
the
ingestion
| @@ -142,8 +170,89 @@ def test_mode_ingest_failure(pytestconfig, tmp_path): | |||
| } | |||
| ) | |||
| pipeline.run() | |||
| try: | |||
There was a problem hiding this comment.
Previously this wasn't actually testing anything... raise_from_status wasn't raising exceptions (we were overriding the implementation on the response object...) so we wouldn't assert anything. Changed up this file a bit so this test is actually testing stuff
datahub-cyborg
bot
added
the
needs-review
| @@ -1470,6 +1473,8 @@ def get_request(): | |||
| response = self.session.get( | |||
| url, timeout=self.config.api_options.timeout | |||
| ) | |||
| if response.status_code == 204: | |||
There was a problem hiding this comment.
can we add a comment here? what's special about 204s?
There was a problem hiding this comment.
204 indicates "No Content", I think mode might be returning empty string in this case which isn't valid json. I can add a comment
| except JSONDecodeError as json_error: | ||
| raise HTTPError( | ||
| f"{json_error.__class__.__name__}: {json_error}" | ||
| ) from json_error |
There was a problem hiding this comment.
this seems extremely janky
let's extract out an ModeRequestErrorTypes = (HTTPError, JSONDecodeError)
and then use that everywhere else in the code e.g.
try:
...
except ModeRequestErrorTypes as error:
...
There was a problem hiding this comment.
Agreed it's a bit sketch. I don't like having to change error handling in like 5 different spots though. What do you think about just catching all exceptions? Seems better in case there's a third one we're somehow missing -- in general it's never good to short circuit ingestion because of a failed request
There was a problem hiding this comment.
the idea for extracting a standard ModeRequestErrorTypes tuple means that we can just reuse that tuple in all our error handling, and we can easily add to the tuple in just one spot
we could also go with catching all exceptions, but it doesn't feel super clean
datahub-cyborg
bot
added
pending-submitter-response
datahub-cyborg
bot
added
needs-review
datahub-cyborg
bot
added
pending-submitter-merge
and removed
needs-review
Checklist