-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
chore: remove druid datasource from the config #19770
chore: remove druid datasource from the config #19770
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19770 +/- ##
==========================================
- Coverage 66.54% 66.27% -0.28%
==========================================
Files 1714 1712 -2
Lines 65102 63964 -1138
Branches 6725 6725
==========================================
- Hits 43321 42389 -932
+ Misses 20069 19863 -206
Partials 1712 1712
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ffc495b
to
bc14df8
Compare
5e718ef
to
0c6597f
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 thought for Superset 2.0 we were just going to mark this as deprecated and actually remove it in Superset 3.0? This change could be seen as too aggressive for some institutions and ideally we should strive not to unnecessarily churn users.
I think the original plan was to remove native Druid support for SIP68, and in trying to get ahead of that work, to mark it as "no longer supported" in 2.0. I believe there is likely going to be something coming before 3.0 that will break this functionality. Do you think that we need to give people more time to switch over to Druid Sql? If absolutely necessary, we can try not to break native Druid as part of the SIP68 work, but I think it's going to be complicated. Or we just hold off on finishing the SIP68 work until 3.0. cc @betodealmeida I figured the only reason that we didn't actually remove the functionality in 2.0 was the time it would take to write this PR. |
@john-bodley I understand the concern here, but at the same time the community has had ample time to prepare for this (link to the SIP #6032 ). Maybe we could float this via the dev list - if there's strong opposition maybe we can postpone until 3.0, but otherwise just rip the bandaid off in 2.0? My personal opinion: the native Druid connector is already missing some key functionality, so I'm actually surprised if it's even possible to use it in a production setting anymore. So I'd vote for removing it in 2.0 or at least making it abundantly clear that it's no longer maintained and may stop functioning all together at any given point after 2.0. |
0c6597f
to
5bf370c
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.
This is awesome! I left a few minor comments.
c707c11
to
386580d
Compare
* remove druid datasource from the config * remove config related references to DruidDatasource * Update __init__.py * Update __init__.py * Update manager.py * remove config related references to DruidDatasource * raise if instance type is not valid
* remove druid datasource from the config * remove config related references to DruidDatasource * Update __init__.py * Update __init__.py * Update manager.py * remove config related references to DruidDatasource * raise if instance type is not valid
In #19262 I added to the UPDATING.md file that the native druid datasource was going to be deprecated in 2.0. After further consideration, I think it will also be more explicit and better for semver to actually remove the config in this major release.
TESTING INSTRUCTIONS
Native Druid NoSQL tests need to be removed.
ADDITIONAL INFORMATION