-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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: new column UUID conflicts in dual write #20460
Conversation
3e985e0
to
f80e701
Compare
Codecov Report
@@ Coverage Diff @@
## master #20460 +/- ##
==========================================
- Coverage 66.67% 66.56% -0.11%
==========================================
Files 1739 1739
Lines 65111 65127 +16
Branches 6896 6896
==========================================
- Hits 43414 43355 -59
- Misses 19948 20023 +75
Partials 1749 1749
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
f80e701
to
04ce15c
Compare
@jinghua-qa If you're free, this could use some basic regression testing around dataset creation and importing. Thank you! |
/testenv up |
@jinghua-qa Ephemeral environment spinning up at http://35.88.160.132:8080. Credentials are |
Tested regression for import and create dataset in ephermal env, LGTM! |
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit 44f0b51)
🏷️ preset:2022.25 |
(cherry picked from commit 44f0b51)
(cherry picked from commit 44f0b51)
Follow up to #20351 where I added some extra checks and lookups for when a column lookup returned
None
on dual write. We're still seeing incidents of the same error:psycopg2.errors.UniqueViolation: duplicate key value violates unique constraint "sl_columns_uuid_key"
intermittently and I was able to repro again in a test when adding in an additional query on the session for existing columns. It seems that the query is triggering an autoflush which is attempting to create a new column when one already exists. I added ano-flush
flag onto any newColumn queries and also made sure to pull in the id of any existing columns when updating it and adding it to the session so that SqlAlchemy will treat it as an update instead of a write.TESTING INSTRUCTIONS
More unit tests, because we don't have a reliable way to repro this issue. But regression testing is necessary. Most issues are happening on import when the dataset already exists and is being overwritten, but for regression, thorough testing of editing and importing datasets that either do or do not exist would be needed.
ADDITIONAL INFORMATION