-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Load Replit DB URL from a file #139
Conversation
Current dependencies on/for this PR:
This comment was auto-generated by Graphite. |
Probably meant |
""" | ||
global db | ||
global db_url | ||
if path.exists("/tmp/replitdb"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't find this file. Is this created exclusively in deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
@@ -16,6 +16,13 @@ async def asyncSetUp(self) -> None: | |||
"""Grab a JWT for all the tests to share.""" | |||
if "REPLIT_DB_URL" in os.environ: | |||
self.db = AsyncDatabase(os.environ["REPLIT_DB_URL"]) | |||
elif "DB_RIDT" in os.environ: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this var set exclusively in deployment? Or for testing only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for testing, I found it to be the quickest way to switch between using JWT or Repl Identity for authing into the DB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Just a couple of questions.
src/replit/database/default_db.py
Outdated
db = Database(db_url) | ||
else: | ||
# The user will see errors if they try to use the database. | ||
db = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this. But there's a problem here. The global db
being updated will not be updated for callers that are importing it via from default_db import db
.
Example counter.py
count = 1
def increment():
global count
count += 2
main.py
from counter import count, increment
print(count)
increment()
print(count)
This will print:
1
1
I suggest instead of making updating the db with a new instance, make the database self-reloading. Or have a db that's exported to the users be a proxy object that defers to a read db instance which handles reloading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be doable to periodically update the db_url https://github.com/replit/replit-py/blob/master/src/replit/database/database.py#L419
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! i think updating the db_url property should work here, let me take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I added a update_db_url
method to the Database
class. Now we initialize the default database and call the method periodically with an up-to-date URL. I tested on an example Repl and it appears to be working fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with a 10 second period. Works fine after the db refresh.
Is a timer in a thread really needed? Couldn't the db class just store the time of last url refresh. Then every db operation check if the last refresh was over an hour ago, if so only refresh the url then. |
Is this ever gonna get merged? I've seen a few users complaining about no support for Repl DB in deployed Python Repls. |
What is the timeline on this update? This is a high priority for me given always-on is unstable now. |
This fix is already in pypi (version 3.3.0 https://pypi.org/project/replit/). Just run |
Just wondering, but shouldn't this PR be merged then? |
Nice |
Why
The JWT used for authenticating into the Replit DB is placed on the Repl as an environment variable on boot and lasts for 30h or so. This is fine for shorter lasting Repls, but if we want to have long-running Repls we need a mechanism to refresh the DB token.
What changed
Let's make it so that we read the DB URL data from a file, which can be refreshed while a program is running. If the file does not exist or we fail to read from it, fall back to the environment variable. We then run this hourly.
Testing
replit
dependency inpyproject.toml
to the latest commit in this branch. The resulting[tool.poetry.dependencies]
section should look something like the following.poetry update