[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-10-16 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-542836941
 
 
   @graceguo-supercat @etr2460 this should be good for another round of testing.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-20 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-533595765
 
 
   Thanks, @graceguo-supercat. Fixing it, sorry for the trouble.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-18 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-532856220
 
 
   Ping @graceguo-supercat @etr2460 


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-13 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-531115915
 
 
   @graceguo-supercat, note that if you want to test this you need to downgrade 
the DB and upgrade again, since I modified the model. Before, I was storing the 
SQL as a reference to a `query` object, but then I hit the bug you found with 
selected text. Now, the SQL is stored explicitly with the state of the tab.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-12 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-531098189
 
 
   @graceguo-supercat, thanks for testing this thoroughly, I had completely 
missed those two cases. I fixed them, and did another look at all the actions — 
everything seems to be covered now.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-12 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-530943398
 
 
   Thanks for testing it, @graceguo-supercat! I'll work on it today!


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-10 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-530062997
 
 
   > In general I think all the error messages could use a little work, mainly 
phrasing them from the perspective an end user would have. These errors would 
reflect a failure to save the query on the backend, and could result in lost 
data, so we should let the user know that.
   
   Thanks, I worked on the error messages making them actionable. Granted, most 
of them simply say "please contact your administrator", but there's not much 
the user can do except for trying saving the query.
   
   > Also, it looks like if the sync to the backend fails, then we don't 
perform the action on the client side, and that we also synchronously wait for 
the backend sync to finish before making the client side change. Maybe instead 
we should be optimistically updating the UI immediately and then firing off the 
request (and reverting the UI back if the query fails). Not sure how much 
latency this adds in actuality.
   
   In my tests there was no perceived latency, since the payload and the 
response are both usually very small. I think it might be more confusing to 
adjust the UI optimistically and revert it when the sync fails.
   
   > As a side note, should we expose a button to the user to manually save the 
query if they see the error message? I worry that people will freak out about 
losing data once they see this new update.
   
   I added a message telling them to click "Save Query" when an error happens. 
I don't think we do a good job in letting users know that they should 
explicitly save the queries they care about, and this was discussed last week 
during the design brainstorm with Cartel. I think later we'll rethink the way 
query organization work, and we can iterate on this.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-09-03 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-527324021
 
 
   @graceguo-supercat that's expected if you don't have a results backend set 
up. In that case, we don't store the results anywhere, and users need to re-run 
the query every time to get the results. If the results backend are configured, 
though, the results are loaded from it.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-31 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-526889346
 
 
   @graceguo-supercat I added code to migrate the query history as well. I'm 
now taking a look at the errors you found with the results backend.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-30 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-526767134
 
 
   > hi @betodealmeida I have one concern for this PR: once this feature is 
released to production, the existed sql lab users open Sql lab, they will not 
see their old query tabs and query history.
   I know the data is still in localStorage. Can we make this 
localStorage=>server-side transition more smooth? some of our sql lab users 
really, really rely on these history data.
   
   @graceguo-supercat this PR **does** migrate the existing state to the 
backend, including the query tabs and table schemas. I'm not 100% sure about 
the query history, I'll test it and if not I'll make sure to include it in the 
migration as well.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-29 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-526401610
 
 
   I added new integration tests, but looks like Travis is not running them. I 
ran them locally and they passed:
   
   ```bash
   $ tox -e cypress-sqllab-backend-persist
   ...
 (Run Finished)
   
   
 SpecTests  Passing  
Failing  Pending  Skipped
 
┌┐
 │ ✔ sqllab/index.test.js  00:1154  
  -1- │
 
└┘
   All specs passed!   00:1154  
  -1-
   
   
   real0m23.016s
   user0m27.370s
   sys 0m5.313s
   
__
 summary 
___
 cypress-sqllab-backend-persist: commands succeeded
 congratulations :)
   ```


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-29 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-526388534
 
 
   @etr2460, do you want to take another stab at reviewing this?
   
   I'm currently working on fixing the failing unit tests, but I'm suspecting 
they might be unrelated and I'm having a hard time reproing them locally. But 
while I figure out the problem, I appreciate if you could do another pass on 
reviewing it, since this is a big PR and there have been other changes to SQL 
Lab in the meantime. Thanks!


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-27 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-525491749
 
 
   I'm fixing the unit tests and adding integration tests.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-20 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-523285273
 
 
   @etr2460, I put all the functionality behind a feature flag. I tested 
locally and it works, and switching the flag on migrates everything to the DB.
   
   I need to add Javascript unit tests and integration tests covering the 
feature flag, but I think this is ready for review.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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



[GitHub] [incubator-superset] betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the backend

2019-08-20 Thread GitBox
betodealmeida commented on issue #8060: SIP-23: Persist SQL Lab state in the 
backend
URL: 
https://github.com/apache/incubator-superset/pull/8060#issuecomment-523276535
 
 
   > For such a major change, I think introducing a feature flag would be 
preferable since it might require significant testing to ensure the solution 
works at scale. I know the PR has a db migration, but it should be harmless to 
do the db migration regardless but feature flag all the other code
   
   Sounds good, I'll do that.


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.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

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