You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/10/26 23:57:16 UTC

[GitHub] [superset] graceguo-supercat opened a new issue #17238: [metrics control] Show error message to user when chart contains bad data

graceguo-supercat opened a new issue #17238:
URL: https://github.com/apache/superset/issues/17238


   **Is your feature request related to a problem? Please describe.**
   This feature request is related to https://github.com/apache/superset/pull/17201.
   
   **Describe the solution you'd like**
   When user creates a chart, they can pick one or more metrics from dataset. With time passing by, or same dataset shared with different groups of users, dataset itself may be changed. In airbnb we found many cases that some metrics used by certain charts, which are not aware by other users, are deleted unintentionally. Later when a user open an old chart which contains these metrics, the whole chart is kind of broken. https://github.com/apache/superset/pull/17201 fixed UI issue. It unblock user to see the chart with valid metrics. But the invalid metric data is still in the Slice entity.
   
   Ideally, Superset should let user know which piece of data is not valid anymore, and ask user to take action to clean up this bad data
   
   **Describe alternatives you've considered**
   https://github.com/apache/superset/pull/17201 fixed UI issue so that unblock user to see the chart with valid metrics.
   
   **Additional context**
   This issue has been in Superset for a long time: https://github.com/apache/superset/issues/16719, https://github.com/apache/superset/issues/14034.
   
   cc @kgabryje @junlincc 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [superset] graceguo-supercat commented on issue #17238: [metrics control] Show error message when chart contains bad data

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #17238:
URL: https://github.com/apache/superset/issues/17238#issuecomment-956395918


   @villebro thanks for the looking this issue.
   
   there are 2 places we can prevent this issue:
   1. from source: we should check which metrics are in use when attempting to delete them. I think this is you suggested.
   2. from target: when bad data already existed in superset data, how Superset should handle the error. this is what i suggested.
   
   I feel both places need some work to fix. From technical point of view, fix 1 is harder, given some companies (like airbnb) already had huge number of charts and dataset. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [superset] villebro commented on issue #17238: [metrics control] Show error message when chart contains bad data

Posted by GitBox <gi...@apache.org>.
villebro commented on issue #17238:
URL: https://github.com/apache/superset/issues/17238#issuecomment-954415090


   If I understand this correctly, the real issue is the fact that we don't check which metrics are in use when attempting to delete them. To check this, we would need to add link table linking charts to metrics, similar to how we do between dashboards and charts or charts and datasets. Then we'd just check if the metric is referenced by charts when trying to delete it, similar to how we do when trying to delete a dataset that's referenced by charts: 
   ![image](https://user-images.githubusercontent.com/33317356/139374221-7272b640-0906-4c71-83fd-36e1510a835b.png)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [superset] graceguo-supercat commented on issue #17238: [metrics control] Show error message when chart contains bad data

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #17238:
URL: https://github.com/apache/superset/issues/17238#issuecomment-956395918


   @villebro thanks for the looking this issue.
   
   there are 2 places we can prevent this issue:
   1. from source: we should check which metrics are in use when attempting to delete them. I think this is you suggested.
   2. from target: when bad data already existed in superset data, how Superset should handle the error. this is what i suggested.
   
   I feel both places need some work to fix. From technical point of view, fix 1 is harder, given some companies (like airbnb) already had huge number of charts and dataset. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [superset] kgabryje commented on issue #17238: [metrics control] Show error message when chart contains bad data

Posted by GitBox <gi...@apache.org>.
kgabryje commented on issue #17238:
URL: https://github.com/apache/superset/issues/17238#issuecomment-954010700


   @junlincc I feel like changing the flow requires some PM and/or designer attention. WDYT?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [superset] villebro edited a comment on issue #17238: [metrics control] Show error message when chart contains bad data

Posted by GitBox <gi...@apache.org>.
villebro edited a comment on issue #17238:
URL: https://github.com/apache/superset/issues/17238#issuecomment-954415090


   If I understand this correctly, the real issue is the fact that we don't check which metrics are in use when attempting to delete them. To check this, we would need to add link table linking charts to metrics (and why not also calculated columns), similar to how we do between dashboards and charts or charts and datasets. Then we'd just check if the metric is referenced by charts when trying to delete it, similar to how we do when trying to delete a dataset that's referenced by charts: 
   ![image](https://user-images.githubusercontent.com/33317356/139374221-7272b640-0906-4c71-83fd-36e1510a835b.png)
   
   I suspect the reason this hasn't been implemented is due to the dual Druid/SQLA datasource model, but if we remove the Druid connector for 2.0, then this will be much easier implement.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org