Skip to content
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

Use SQLite to store torrent checkpoints data #5669

Closed
ichorid opened this issue Oct 22, 2020 · 11 comments
Closed

Use SQLite to store torrent checkpoints data #5669

ichorid opened this issue Oct 22, 2020 · 11 comments

Comments

@ichorid
Copy link
Contributor

ichorid commented Oct 22, 2020

Currently, we store torrent data and stats in separate files in the dlcheckpoints dir. Libtorrent never touches any of these. Instead, we mediate its access through Python code.

Moving to use PonyORM backed SQLite storage for torrents will save us a lot of hassle regarding file access synchronization/persistence, the kind of things affecting e.g. #5615. Also, it will enable simpler and tighter integration with Channels database, resulting in faster UI response.

What do you think, guys?

@ichorid
Copy link
Contributor Author

ichorid commented Oct 22, 2020

@egbertbouman , we need your opinion on this, as the person who touched the wrapper/checkpoint files most closely?

@ichorid ichorid changed the title Use SQLite to store torrent data Use SQLite to store torrent checkpoints data Oct 22, 2020
@egbertbouman
Copy link
Member

I don't really see how the issue you referenced is tied to the fact that we're storing the metadata/stats in files. Since we have to write the metadata/stats ourselves in both situations, the incorrect data will just be in the database instead. I also don't get the benefit in terms of hassle.

If there is a performance improvement by using the database, I think it's worth investigating this. Just for my understanding. What information that we're currently getting from files, will be faster by moving it to the database?

@ichorid
Copy link
Contributor Author

ichorid commented Oct 22, 2020

AFAIK, in our current design, we store the upload/download counters in-memory. We dump the data on disk only when closing Tribler, right? When opening Tribler, we load all the counters back into memory. This may cause losing ratio counters if Tribler was shut down incorrectly.

By using SQLite we can solve persistence/shutdown problems with dlcheckpoints, while simultaneously removing a lot of utility code for e.g. managing our own file format for config files. Also, this will enable us to cache torrent bencoded dicts in the DB, as we did before in times of lmdb.

@xoriole
Copy link
Contributor

xoriole commented Oct 22, 2020

I like the idea of persisting stats but if something like this #5252 happens the user will lose their downloads (list).

@egbertbouman
Copy link
Member

The stats are collected by libtorrent itself (it's part of the resumedata). When checkpointing, this data is stored in the checkpoint file, along with the metadata.

@ichorid
Copy link
Contributor Author

ichorid commented Oct 22, 2020

The stats are collected by libtorrent itself (it's part of the resumedata). When checkpointing, this data is stored in the checkpoint file, along with the metadata.

You mean, the stats are stored in binary form and read directly by Libtorrent, right?

@egbertbouman
Copy link
Member

Correct, when we resume a download after a restart, we give to metadata/resumedata back to libtorrent:

metainfo = self.tdef.get_metainfo()
torrentinfo = lt.torrent_info(metainfo)
atp["ti"] = torrentinfo
if resume_data and isinstance(resume_data, dict):
# Rewrite save_path as a global path, if it is given as a relative path
if b"save_path" in resume_data and not path_util.isabs(ensure_unicode(resume_data[b"save_path"], 'utf8')):
resume_data[b"save_path"] = self.state_dir / ensure_unicode(resume_data[b"save_path"], 'utf8')
atp["resume_data"] = lt.bencode(resume_data)

@synctext
Copy link
Member

Not an expert on performance or balance design as discussed above; but....
It is vital that Tribler keeps on going, it should never crash or refuse to work. Youtube, TikTok or Spotify dont need a "repair database" button. Please enables safety checks. Losing a few bytes in the latest counters is not important compared to guarding database integrity.

Example of world-class reliability engineering by BBC

@ichorid
Copy link
Contributor Author

ichorid commented Oct 23, 2020

Firefox uses SQLite internally. I guess if it is good for them, it is good for us too...

@ichorid
Copy link
Contributor Author

ichorid commented Oct 25, 2020

@synctext :

Simplicity

@qstokkink
Copy link
Contributor

We'll follow the advice of @egbertbouman and not do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants