-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Destination Databricks: Add generation_id+sync_id #40689
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
3c92fd1 to
5ea3ddb
Compare
4aa4151 to
d7589bf
Compare
6ceee39 to
a7384fd
Compare
d7589bf to
ee4d4d4
Compare
a7384fd to
aa5f4d5
Compare
ee4d4d4 to
0c533f1
Compare
aa5f4d5 to
3212d3b
Compare
0c533f1 to
100d7ad
Compare
3212d3b to
035032d
Compare
100d7ad to
8d8804d
Compare
f8228c3 to
06ee74e
Compare
a08885a to
859842c
Compare
68d8bb2 to
3314670
Compare
859842c to
76745ef
Compare
b4d4784 to
6dfd22e
Compare
43e5322 to
cfa19a5
Compare
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 could be convinced to undo this, or even just use the actual constant names evereywhere, but defining consts to alias other constants feels weird - I replaced those with just import aliases.
(also, fyi constants was previously an import alias for JavaBaseConstants)
92f605e to
eb7e421
Compare
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.
see DatabricksSqlGenerator for more explanation of what these constants were / why I'm removing them
49c5c79 to
f7c724a
Compare
| # minimum 3 minutes timeout required during parallel workload on our small warehouse | ||
| JunitMethodExecutionTimeout=3 m | ||
| # increased timeout required during parallel workload on our small warehouse | ||
| JunitMethodExecutionTimeout=10 m |
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.
saw some timeouts in CI. I'm pretty sure that's a consequence of having two stacked PRs (i.e. our cluster just isn't beefy enough to run multiple PRs' tests at the same time); if I push a single branch at a time, we don't hit the timeout.
(not opinionated on whether we should just increase our cluster size, I don't know how databricks bills us)
| @@ -1,8 +0,0 @@ | |||
| {"id1": 1, "id2": 100, "updated_at": "2023-01-01T01:00:00.000000Z", "array": ["foo"], "struct": {"foo": "bar"}, "string": "foo", "number": 42.1, "integer": 42, "boolean": true, "timestamp_with_timezone": "2023-01-23T12:34:56.000000Z", "timestamp_without_timezone": "2023-01-23T12:34:56", "time_with_timezone": "12:34:56Z", "time_without_timezone": "12:34:56", "date": "2023-01-23", "unknown": {}, "_airbyte_extracted_at": "2023-01-01T00:00:00.000000Z", "_airbyte_meta": {"changes": []}} | |||
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.
there's no v1v2 migration test (since we never implemented the v1->v2 migration to begin with), so deleting these files
ed23537 to
4b5fff5
Compare
johnny-schmidt
left a comment
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.
LGTM, one question
...icks/src/main/kotlin/io/airbyte/integrations/destination/databricks/DatabricksDestination.kt
Show resolved
Hide resolved
230bd8f to
c5d7f91
Compare
|
@airbytehq/destinations I forgot to write the breaking change text 🤦 can I get a review on the upgrade guide stuff? |
c5d7f91 to
c019d19
Compare

closes https://github.com/airbytehq/airbyte-internal-issues/issues/8533. We'll release this as a breaking change, so no migration necessary.
stacked on #40567 to pull in a fix for one of the sqlgenerator test cases.