john-bodley commented on pull request #18181: URL: https://github.com/apache/superset/pull/18181#issuecomment-1040827811
@michael-s-molina thanks for addressing a long outstanding issue with Superset URLs, however a number of us at Airbnb have a few comments/concerns regarding this work—we're currently reverting this in our weekly release. Specifically: 1. It seems like using a (semi-)persistent store for tackling the problem of long URLs seems someone akin to using a hammer to crack a nut—even though it works there may likely be simpler methods which don't require persistence of state. For example was the ideal of simply compressing the JSON form data considered? 2. Flask caching (regardless of the backing store) is primarily used as a cache—ideally with TTL functionality to prevent overflow. Using a cache for (semi-)persistent storage seems somewhat of an anti-pattern and likely violates the implicit understanding of how Superset's cache is used, i.e., in theory you should be able to obliterate the entire cache without impacting the _functionality_ of the application (_performance_ will clearly be degraded). If we plan on using a store to remedy the problem we likely should lean on Superset's metadata database—augmenting the shortened URL functionality. 3. It seems like there are aspects of this change which are still in flux, e.g. backwards compatibility etc., and thus I would consider this experimental in nature and likely should have been behind a feature flag (and defaulted to disabled) whilst the design/implementation was hardened. 4. I realize that the ship has somewhat sailed, but per [SIP-0](https://github.com/apache/superset/issues/5602) it mentions, > Any of the following should be considered a major change: > > - Any major new feature, subsystem, or piece of functionality > - **Any change that impacts the public interfaces of the project** > > What are the "public interfaces" of the project? All of the following are public interfaces that people build around: > > - Visualization types > - **Form data for saved dashboards and charts** > - ... which would indicate that this feature likely should have been presented as a SIP to allow for discussion regarding design/implementation and sign off from the community. -- 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-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
