-
Notifications
You must be signed in to change notification settings - Fork 14k
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(druid): Delete obsolete Druid NoSQL slice parameters #24737
fix(druid): Delete obsolete Druid NoSQL slice parameters #24737
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24737 +/- ##
=======================================
Coverage 68.89% 68.89%
=======================================
Files 1901 1901
Lines 73927 73927
Branches 8183 8183
=======================================
Hits 50932 50932
Misses 20874 20874
Partials 2121 2121
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
f8ae048
to
007baa1
Compare
if updated: | ||
slc.params = json.dumps(params) | ||
except Exception: | ||
pass |
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.
can we add a log here to see the exception
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.
@hughhhh good idea.
if updated: | ||
slc.query_context = json.dumps(query_context) | ||
except Exception: | ||
pass |
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 as comment above
bbfe5f3
to
52398c7
Compare
@hughhhh I've addressed your comments. The exceptions are logged in the form:
|
52398c7
to
5ad9106
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.
LGTM
(cherry picked from commit 4c5ada4)
SUMMARY
This PR addresses an issue raised by @padbk in #24581 (comment) related to sending an alert/revert via a CSV. The actual error was rather cryptic:
Specifically the RESTful
/api/v1/chart/{pk}/data/?format=csv
API endpoint was failing with the following error,which turned out to be related to #23997, i.e., the referenced PR (which I authored) didn't remove the now obsolete parameters from the
slices
table. This PR remedies said issue.I used Airbnb's vast corpus (consisting of ~ 150k charts) to identify if/where the following form-data key were persisted to the database:
druid_time_origin
having_druid
having_filters
granularity
I wasn't able to find
having_filters
anywhere, but it was somewhat 😢 to see that thedruid_time_origin
andgranularity
fields were actually present in three separate places:slices.params
columnform_data
field of theslices.query_context
column.queries
field of theslices.query_context
column.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI and added/tested the DB migration. After running the migration I then confirmed that that I was successfully able to generate a report sent as a CSV.
ADDITIONAL INFORMATION