-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Normalization Summaries table and read/write methods #16655
Conversation
@Override | ||
public List<NormalizationSummary> getNormalizationSummary(final Long attemptId) throws IOException { | ||
return jobDatabase | ||
.query(ctx -> ctx.select(DSL.asterisk()).from(DSL.table("normalization_summaries")).where(NORMALIZATION_SUMMARIES.ATTEMPT_ID.eq(attemptId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use the NORMALIZATION_SUMMARIES
constant for the table name.
.withFailures(record.get(NORMALIZATION_SUMMARIES.FAILURES, String.class) == null ? null | ||
: Lists.newArrayList(mapper.readValue(String.valueOf(record.get(NORMALIZATION_SUMMARIES.FAILURES)), FailureReason[].class))); | ||
} catch (final JsonProcessingException e) { | ||
return new NormalizationSummary().withStartTime(record.get(NORMALIZATION_SUMMARIES.START_TIME).toInstant().toEpochMilli()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: add a comment (and possible a debug log statement for the exception) as to why we are returning this default normalization summary when the JSON exception occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added a comment - let me know what you think. I thought we should rescue this error since nobody is actually reading this field from the app so I don't want this read to cause an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also throw an exception in order to have real failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rescued the exception since nobody is reading this field from the app
5f50d50
to
6ce742a
Compare
return new NormalizationSummary().withStartTime(record.get(NORMALIZATION_SUMMARIES.START_TIME).toInstant().toEpochMilli()) | ||
.withEndTime(record.get(NORMALIZATION_SUMMARIES.END_TIME).toInstant().toEpochMilli()) | ||
.withFailures(record.get(NORMALIZATION_SUMMARIES.FAILURES, String.class) == null ? null | ||
: Lists.newArrayList(mapper.readValue(String.valueOf(record.get(NORMALIZATION_SUMMARIES.FAILURES)), FailureReason[].class))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we should use List.of
to enforce immutability
And could we extract the deserialization of the Failures in its own method?
.withFailures(record.get(NORMALIZATION_SUMMARIES.FAILURES, String.class) == null ? null | ||
: Lists.newArrayList(mapper.readValue(String.valueOf(record.get(NORMALIZATION_SUMMARIES.FAILURES)), FailureReason[].class))); | ||
} catch (final JsonProcessingException e) { | ||
return new NormalizationSummary().withStartTime(record.get(NORMALIZATION_SUMMARIES.START_TIME).toInstant().toEpochMilli()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this also throw an exception in order to have real failure?
}; | ||
try { | ||
return List.of(mapper.readValue(String.valueOf(record.get(NORMALIZATION_SUMMARIES.FAILURES)), FailureReason[].class)); | ||
} catch (final JsonProcessingException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to rethrow this exception, we are doing it for the IOException, the API will return a 500 error instead of a 200 with a null list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf1101a
to
9345169
Compare
FE tests are flaky: https://airbytehq-team.slack.com/archives/C03088BTMFC/p1663186255780899 |
* master: (200 commits) 🪟 🧹 Display returned error messages on replication view (#16280) 🎉 Source mixpanel: Use "Retry-After" header for backoff (#16770) 🐛 Source google ads: mark custom query fields required (#15858) 🪟 🔧Remove useRouter hook (#16598) CDK: improve TypeTransformer to convert simple types to array of simple types (#16636) CDK: TypeTransformer - warning message more informative (#16695) Source MySQL: Add Python SAT to detect backwards breaking changes (#16445) remove eager (#16756) bump com.networknt:json-schema-validator to latest version (#16619) Remove Cloud from Kafka docs (#16753) Normalization Summaries table and read/write methods (#16655) comment out flaky test suite while it is being investigated (#16752) Update ConfigRepository to read protocol version (#16670) Use LOG4J2 to wrap connectors logs to JSON format (#15668) Update connector catalog (#16749) 🪟 🎨 Remove feedback modal from UI (#16548) Add missing env var for Kube overlays (#16747) Prepare for React v18 upgrade (#16694) 🪟 🐛 Fix direct job linking to work with pagination (#16517) Fix formatting (#16743) ...
* Add migration to create normalization summaries table - read/write methods for normalization summary
* Add migration to create normalization summaries table - read/write methods for normalization summary
Normalization Summaries are saved in JSON in the
output
column of theattempts
table. Since we need to query this data, this PR moves Normalization Summaries to its own table and adds read/write methods.