-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor daemon with memory-safe loop and logging #40
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
base: main
Are you sure you want to change the base?
Conversation
|
@varmar05 To me this looks ok. I should not be really necessary to manually call the The logging seems to impove with this as well. |
varmar05
left a comment
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.
Thanks @Ojaybee for your contribution. I like the logging improvement and bit of refactoring. However I would like to understand more the memory part.
| logger.info("Sync complete.") | ||
|
|
||
| # Force garbage collection | ||
| gc.collect() |
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.
Do we know what was the issue with memory (maybe we could fix the source of the issue)? As gc should be automatic we should probably know why to force it.
| sys.exit(1) | ||
|
|
||
| print("Logging in to Mergin...") | ||
| logger.info("Logging in to Mergin...") |
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.
I suggest to update Mergin... to Mergin Maps server ... in all occurrences for better clarity
| if delta.total_seconds() < 3600: | ||
| logger.info("Refreshing Mergin auth token...") | ||
| mc = create_mergin_client() | ||
| except Exception as e: |
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.
This seems to me too broad, can you narrow it down to MerginClient error or something similar?
Refactors media_sync_daemon.py to eliminate unbounded memory growth by rewriting the main loop.
Tested on Lightsail t3.nano with persistent background sync.
Note: Block I/O still increasing unchecked.