Enable to use a SQL DB as a storage backend #11

miraculixx · 2022-05-23T23:10:29Z

closes #9

AbdealiLoKo · 2022-05-26T07:32:25Z

I have merged the PR for github-actions - Could you rebase this PR based on that ?
Also, could you help me understand why do you use dataset and not sqlalchemy directly here ?

dataset seems to need a bunch of other dependencies which we could avoid - https://github.com/pudo/dataset/blob/master/setup.py#L32

AbdealiLoKo · 2022-05-26T07:34:56Z

configurable_http_proxy/dbstore.py

+
+    def __init__(self):
+        super().__init__()
+        db_url = os.environ.get("CHP_DATABASE_URL", self.default_db_url)


Instead of using environment variables ? Can we get the configs for this from the jupyter config file ?
For example how the other jupyterhub proxy configs have it: https://jupyterhub-traefik-proxy.readthedocs.io/en/latest/toml.html#example-setup

I have merged the PR for github-actions - Could you rebase this PR based on that ? Also, could you help me understand why do you use dataset and not sqlalchemy directly here ?

dataset seems to need a bunch of other dependencies which we could avoid - https://github.com/pudo/dataset/blob/master/setup.py#L32

The rationale for using dataset is that it reduces the effort to implement this feature by about 90%. dataset's core benefit is that it abstracts-away all the sqlalchemy nuts & bolts, in particular we can use Python-dicts as input/output of the DB. Doing the same using sqlalchemy directly would add quite a bit of code for db handling, which dataset allows us to avoid.

Apart from sqlalchemy, dataset adds two dependencies, alembic and banal. To make this feature operationally useful, it needs to support db upgrades (migrations), so alembic is required anyway. The additional dependency on banal seems a very small price to pay for that.

Can we get the configs for this from the jupyter config file ?

IMHO jupyter config files should be a concern of Jupyter, not of chp (if chp depends on jupyter configs there will be a dependency chp => jupyter, which would not be useful in my mind).

On the jupyter configs - makes sense
I guess I was more thinking of adding CLI arguments instead of env variables to be consistent with the other configs we use in configurable-http-proxy

AbdealiLoKo · 2022-05-26T07:46:04Z

configurable_http_proxy/dbstore.py

+
+
+class TableTrie:
+    # A databased URLTrie-alike


Could you add a docstring here explaining how the trie structure is being saved in the DB with an example ?
I was trying to go through the code but reallized I was unable to figure out the table structure we are using (probably due to my lack of knowledge on dataset).

Could you add a docstring here explaining how the trie structure is being saved in the DB with an example ?

Done. On a side note, it might be useful to add a docstring to the trie.URLtrie class because it isn't obvious either ;-)

AbdealiLoKo · 2022-05-26T07:50:08Z

README.md

@@ -27,6 +27,7 @@ The following items are supported:
 - Customizable storage backends
 - PID file writing
 - Logging
+- Configurable storage backend


I think this is an awesome feature and really useful.

We should highlight it by making 3 sections here:

Features supported (and compatible) with nodejs configurable-http-proxy

Additional features not available in nodejs configurable-http-proxy

Features from nodejs configurable-http-proxy not supported (yet)

Updated the README accordingly

miraculixx · 2022-05-28T15:40:27Z

I have merged the PR for github-actions - Could you rebase this PR based on that?

🆗 Done

- add sqlastore.DatabaseStore and unittests - update API unittests for use in database tests - update README

AbdealiLoKo · 2022-11-10T14:24:38Z

@miraculixx I use rebase merges to maintain a linear commit history instead of merge commits
So, I have rebased your branch on main and pushed

I made some very small modifications in class names in tests (I avoid using _ in the class names)
Also in some places the number of / in sqlite:///chp.sqlite was 2 instead of 3
And modified the readme a little and formatted it using markdown / prettier

Got the tests to pass and I am good to merge it

AbdealiLoKo · 2022-11-10T14:25:03Z

Thanks a lot for the contribution - this is super helpful @miraculixx !

SMHari · 2023-01-19T09:16:22Z

Hi @miraculixx @AbdealiLoKo
is this sqla package still valid?
I noticed that the version of sqla is 0.0.0 from PyPI index and there seems to be no active versions being maintained in GitHub

any alternatives that can be used?
https://pypi.org/project/sqla/

AbdealiLoKo · 2023-01-24T07:19:32Z

Hi @SMHari The sqla mentioned here is an extra available with configurable-http-proxy
Not the sqla package on pypi

I.e. you need to run pip install configurable-http-proxy[sqla] which installs configurable-http-proxy with sqlalchemy dependencies
https://github.com/corridor/configurable-http-proxy/blob/main/setup.py#L23

Ref: Example 4 in https://pip.pypa.io/en/stable/reference/requirement-specifiers/?highlight=security#examples

SomeProject[foo, bar]
# OR:
requests[security]

SMHari · 2023-01-24T17:19:34Z

Hi @AbdealiLoKo
thanks for checking

But I'm getting the below warning while trying to do the same

WARNING: configurable-http-proxy 0.2.3 does not provide the extra 'sqla'

But I still tried starting the CHP with the storage-backend argument,
ie: configurable-http-proxy --storage-backend configurable_http_proxy.dbstore.DatabaseStore

it is failing because of the same

ModuleNotFoundError: No module named 'configurable_http_proxy.dbstore'

Am I missing anything?

AbdealiLoKo · 2023-01-25T05:45:48Z

Hm, looks like we haven't released this to pypi yet.
@indiVar0508 could you release this to pypi

AbdealiLoKo · 2023-01-25T11:32:19Z

We just released it @SMHari
To use it in your environment, please do:

pip uninstall configurable-http-proxy
pip install configurable-http-proxy[sql]

NOTE that I have changed it from sqla -> sql to avoid confusion the the pypi library sqla (As the previous name was never released)

SMHari · 2023-01-25T14:45:38Z

Cool
Thanks, @AbdealiLoKo
that is working fine

But I'm using this py CHP with my jupyterhub as an alternative to nodejs CHP
So, whenever, I try to open a notebook, I'm seeing the below exception in proxy logs, which is not the case for the other CHP. As a result, the notebook keeps loading

`Traceback (most recent call last):
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/websocket.py", line 1086, in write_message
    fut = self._write_frame(True, opcode, message, flags=flags)
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/websocket.py", line 1061, in _write_frame
    return self.stream.write(frame)
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/iostream.py", line 530, in write
    self._check_closed()
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/iostream.py", line 1019, in _check_closed
    raise StreamClosedError(real_error=self.error)
tornado.iostream.StreamClosedError: Stream is closed

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/websocket.py", line 635, in _run_callback
    result = callback(*args, **kwargs)
  File "/export/apps/python/3.10/lib/python3.10/site-packages/configurable_http_proxy/handlers.py", line 414, in on_message
    self.ws_client.write_message(message)
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/websocket.py", line 1500, in write_message
    return self.protocol.write_message(message, binary=binary)
  File "/export/apps/python/3.10/lib/python3.10/site-packages/tornado/websocket.py", line 1088, in write_message
    raise WebSocketClosedError()
tornado.websocket.WebSocketClosedError`

AbdealiLoKo · 2023-01-25T16:41:46Z

Hi @SMHari moving this to an issue so we can track it

SMHari · 2023-01-25T16:58:56Z

Sure @AbdealiLoKo
Appreciate your help :)

Whether this will be actively addressed or might take some time ?

miraculixx force-pushed the enable-sqla-backend branch 2 times, most recently from 2c69170 to ff7570a Compare May 23, 2022 23:49

miraculixx changed the title ~~Enable sqla backend~~ Enable to use a SQL DB as a storage backend May 23, 2022

miraculixx force-pushed the enable-sqla-backend branch from ff7570a to ab93c02 Compare May 23, 2022 23:58

miraculixx mentioned this pull request May 24, 2022

Support a db backend for high availability scenarios #9

Closed

miraculixx force-pushed the enable-sqla-backend branch 7 times, most recently from 2d756f7 to d7417da Compare May 25, 2022 13:40

AbdealiLoKo reviewed May 26, 2022

View reviewed changes

miraculixx force-pushed the enable-sqla-backend branch 3 times, most recently from bc5f0e6 to 7dc0ace Compare May 28, 2022 15:07

miraculixx force-pushed the enable-sqla-backend branch from 7dc0ace to dd6e633 Compare June 13, 2022 09:43

miraculixx mentioned this pull request Nov 10, 2022

Restrict log #15

Closed

AbdealiLoKo force-pushed the enable-sqla-backend branch from bb12fd7 to 343acac Compare November 10, 2022 14:13

Add SQLAlchemy storage backend

9a06c81

- add sqlastore.DatabaseStore and unittests - update API unittests for use in database tests - update README

AbdealiLoKo force-pushed the enable-sqla-backend branch from 343acac to 9a06c81 Compare November 10, 2022 14:22

AbdealiLoKo merged commit d9f1469 into corridor:main Nov 10, 2022

AbdealiLoKo mentioned this pull request Jan 25, 2023

Error when using CHP-py with jupyterhb #16

Open

Enable to use a SQL DB as a storage backend #11

Enable to use a SQL DB as a storage backend #11

Uh oh!

Conversation

miraculixx commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbdealiLoKo commented May 26, 2022

Uh oh!

AbdealiLoKo May 26, 2022

Choose a reason for hiding this comment

Uh oh!

miraculixx May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miraculixx May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbdealiLoKo Nov 10, 2022

Choose a reason for hiding this comment

Uh oh!

AbdealiLoKo May 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miraculixx May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AbdealiLoKo May 26, 2022

Choose a reason for hiding this comment

Uh oh!

miraculixx May 28, 2022

Choose a reason for hiding this comment

Uh oh!

miraculixx commented May 28, 2022

Uh oh!

AbdealiLoKo commented Nov 10, 2022

Uh oh!

AbdealiLoKo commented Nov 10, 2022

Uh oh!

SMHari commented Jan 19, 2023

Uh oh!

AbdealiLoKo commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SMHari commented Jan 24, 2023

Uh oh!

AbdealiLoKo commented Jan 25, 2023

Uh oh!

AbdealiLoKo commented Jan 25, 2023

Uh oh!

SMHari commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbdealiLoKo commented Jan 25, 2023

Uh oh!

SMHari commented Jan 25, 2023

Uh oh!

Uh oh!

miraculixx commented May 23, 2022 •

edited

Loading

miraculixx May 28, 2022 •

edited

Loading

miraculixx May 28, 2022 •

edited

Loading

AbdealiLoKo May 26, 2022 •

edited

Loading

miraculixx May 28, 2022 •

edited

Loading

AbdealiLoKo commented Jan 24, 2023 •

edited

Loading

SMHari commented Jan 25, 2023 •

edited

Loading