Skip to content
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

fix(ingest): raise errors in more cases in rest sink #2638

Closed
wants to merge 3 commits into from

Conversation

gabe-lyons
Copy link
Contributor

Rest sink would occasionally swallow HTTP errors. Now, if they get a 404 say, the rest sink outputs:

[2021-06-03 11:27:21,460] ERROR    {datahub.ingestion.run.pipeline:52} - failed to write record with workunit file://./examples/mce_files/bootstrap_mce.json:36 with 404 Client Error: Not Found for url: http://localhost:9002/entities?action=ingest and info {}

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable)

raise OperationalError(
"Unable to parse response from ingestion sink",
{"message": str(parse_exception)},
) from e
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're in except HTTPError as e:, we know that response.raise_for_status() will certainly throw here - probably want to remove that line since we already have it in e

We're actually not interested in the parse_exception since it just means that the response body wasn't json. Instead, we should set message to the response.text itself

@xiphl
Copy link
Contributor

xiphl commented Jun 10, 2021

I encountered a case where I fed in a json MCE file to rest api. I then checked mysql, the entries did not make it into the database; but the entries were recorded into Elasticsearch.
I was wondering if this is related...
repeating the steps did the trick though.

@jjoyce0510
Copy link
Collaborator

@xiphl This is very concerning, and we've not seen it before. Please let us know if you experience this issue again

@shirshanka
Copy link
Contributor

Closing as #2800 supercedes this.

@shirshanka shirshanka closed this Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants