HUE-8740 [sql] Add create_session to sqlalchemy & cache engine.

Review Request #13783 — Created April 3, 2019 and updated

commit e4736aad74c9ab7c548db30073ab7fe9cdeee73e
Author: jdesjean <>
Date:   Wed Apr 3 11:55:50 2019 -0700

    HUE-8740 [sql] Add create_session to sqlalchemy & cache engine.

:100644 100644 bbdf91db4b... 63ca01538a... M	desktop/libs/notebook/src/notebook/
:100644 100644 a944ef9d9e... bcfe1b65ab... M	desktop/libs/notebook/src/notebook/connectors/

  • 0
  • 0
  • 1
  • 3
  • 4
Description From Last Updated
Review request changed
  1. What is the goal of caching? When we have:
    - Hue server only: keep open for downloading the result later without redoing the query?
    - Hue server + Task server: still needed?

    1. 1) The engine has a connection pool to the DB. Creating a new engine on every request means we shutdown / open connections on every request. We need to cache this just like we have a connection for HS2.
      2) This also handles the left assist / table browser which currently don't work at all. (e.g. we'd have to create_engine for each of those methods and fix notebook api to carry the credentials on each of those methods.)

  2. desktop/libs/notebook/src/notebook/ (Diff revision 2)

    Could merge back these 2 too

    Could also remove for now

  3. We probably are going to leak stuff, could we use Django cache framework and TTL?


    1. I'm ok with using some API for this, but none of the current django implementation would work for this case. The engine cannot be shared accross processes, so we'd have to create our own implementation.

  4. Seems wrong to have one engine cache for everything?

    1. 1) The previous implementation did not support multiple credentials.
      2) Nothing else in Hue support multi credentials (one for each user).

      If there's only one set of credentials, then there should only be one engine.

  5. Do we really need those here and below?

    (for me it means more that we forgot to call create_session())

    1. This "should" not happen, but it's a reasonable error to throw if it does. Currently SessionExpired is correctly handled by the frontend in execute.