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

Msiega/log unexpected fields #21922

Merged
merged 4 commits into from
Jan 26, 2023
Merged

Conversation

mfsiega-airbyte
Copy link
Contributor

What

Log when we see fields that aren't in the catalog.

How

  • Make a map of stream -> fields in the catalog.
  • For each record, check if any of the fields are missing from the catalog; save these.
  • At the end of the sync, log any unexpected fields.

@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets January 26, 2023 16:02 — with GitHub Actions Inactive
@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets January 26, 2023 16:02 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 26, 2023

Airbyte Code Coverage

File Coverage [90.15%] 🍏
DefaultReplicationWorker.java 90.15% 🍏
Total Project Coverage 23.99%

}
}
} else {
throw new RuntimeException(String.format("Unexpected data in record: %s", data));
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a posibility to have a valid non object here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean it should not happen, but if that happen something is going to break in the code in the if block, better to test and throw a more specific exception.
We could use a more specific error message though "Expecting object in record data, got '%s' instead" or something like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(@malikdiarra you had a comment on the same block)

According to the Airbyte Protocol the data property of a record message must be a JSON of object type, so in theory no.

However, on second thought I don't think we should be throwing here in any case - that will basically crash the whole sync, and that's why we have the validation step (which doesn't throw, but rather builds a list of errors).

Copy link
Contributor

@malikdiarra malikdiarra left a comment

Choose a reason for hiding this comment

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

This looks good to me. How easy is it to test this block of code before we release it?

}
}
} else {
throw new RuntimeException(String.format("Unexpected data in record: %s", data));
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean it should not happen, but if that happen something is going to break in the code in the if block, better to test and throw a more specific exception.
We could use a more specific error message though "Expecting object in record data, got '%s' instead" or something like this.

@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets January 26, 2023 17:15 — with GitHub Actions Inactive
@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets January 26, 2023 17:15 — with GitHub Actions Inactive
@mfsiega-airbyte mfsiega-airbyte enabled auto-merge (squash) January 26, 2023 17:22
@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets January 26, 2023 17:24 — with GitHub Actions Inactive
@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets January 26, 2023 17:24 — with GitHub Actions Inactive
@mfsiega-airbyte mfsiega-airbyte merged commit 1475d6e into master Jan 26, 2023
@mfsiega-airbyte mfsiega-airbyte deleted the msiega/log-unexpected-fields branch January 26, 2023 17:55
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.

3 participants