Tribler / tribler

Privacy enhanced BitTorrent client with P2P content discovery
https://www.tribler.org
GNU General Public License v3.0
4.84k stars 449 forks source link

Channels Retirement #7669

Closed drew2a closed 3 weeks ago

drew2a commented 1 year ago

Channels Retirement Plan:

Core Side:

Data Side (migration):

GUI Side:

Post-implementation, users will encounter an updated GUI:

image

On the core side, the PopularityCommunity will inherit the search feature from the ChannelCommunity.

egbertbouman commented 12 months ago

Just in case anyone is curious, I have a branch here. Please bear in mind that this branch was meant to see if we could easily remove the channels, and shouldn't be considered stable. It took about 3-4 days to make this, while losing 7500 LoC.

qstokkink commented 11 months ago

@egbertbouman In order to get a PR out of your branch, it seems like these are the remaining TODOs:

  1. Rebase to Tribler/main.
  2. Remove commented-out code.
  3. Restore tests for torrent_metadata.py.

Is that correct?

By the way, I'm not trying to solve ALL the bullet points listed in O.P. in a single PR. I'll leave the rest for (a) follow up PR(s).

egbertbouman commented 11 months ago

Sounds right, although I'm not sure how many of the tests you can restore from the original test_torrent_metadata.py since ChannelMetadata/CHANNEL_TORRENT was removed (all that's left is TorrentMetadata/REGULAR_TORRENT). Having said that, there are probably some things untested for TorrentMetadata. The serialization stuff is getting tested in test_store.py.

One thing to note (for potential future PRs) is that I basically didn't touch the database. The main db table is still called ChannelNode (I added TorrentMetadata._table_ = "ChannelNode"), and all the columns from previous db versions will still be there.

qstokkink commented 11 months ago

@egbertbouman Cool, thanks 👍 That sounds like it should be a relatively quick* job then ("famous last words"). I'll get started on this now then.

*EDIT: 45 minutes of merge conflicts 🙂

qstokkink commented 11 months ago

The initial cut has now been merged in #7726. I'll move on to the (hopefully) smaller points now.

qstokkink commented 11 months ago

The next step will be to merge PopularityCommunity, RemoteQueryCommunity and VersionCommunityMixin. I guess we can rename it to ContentDiscoveryCommunity as posted in O.P.

After this I'll hand off this issue to @kozlovsky to merge the databases into one component.

qstokkink commented 11 months ago

The PopularityCommunity has now been renamed to ContentDiscoveryCommunity. The multiple inheritance structure has been collapsed into a single community, all LOC now have test coverage, and some dead code was removed.

I'll start spending some time on doing "aftercare" for the two big PRs (in the form of fixing bugs).

The remaining "metadatastore -> Tribler database" refactoring(s) will remain pending until our database expert @kozlovsky comes back from vacation.

qstokkink commented 11 months ago

The "search" REST endpoint is part of metadata_store, which is being moved as part of this issue. If I start working on https://github.com/Tribler/tribler/issues/7632#issuecomment-1792247487 right now, this would lead to nasty merge conflicts. To avoid that, a small change to the roll-out plans: I'll perform the physical move of the files in metadata_store first.

After the move, the "metadatastore -> Tribler database" integration will still need to happen but it won't interfere too much with the other work because the relevant files will have been moved to a stable location.

Detailed plan

Main source moves (relative to the src/tribler/core/components folder):

Unit tests should be moved to the appropriate locations. Components and their settings should be merged into the appropriate components. The contents utils.py should be moved into the test configurations (if they are still used - this is probably dead code).

qstokkink commented 10 months ago

With the merge of #7773, I will (try to again) partially pull out of this issue to focus on https://github.com/Tribler/tribler/issues/7632#issuecomment-1792247487

As I laid out before, I'll wait for @kozlovsky to come back from vacation to make sure that the final merge of the TriblerDatabase and MetadataStore happens successfully. After that merge, this isssue is resolved.

qstokkink commented 1 month ago

@egbertbouman when you're back from vacation: if the upgrader script is now using explicit tables anyway (as proposed in #8163) it might be easier to do the final database file merge before the 8.0 release and handle this in the 7.14 -> 8.0 upgrade. This avoids a secondary method to upgrade from 8.0 format to 8.1 (or whatever version has the merge).

That said, this will be a lot of work again and it might cause instability just before the shiny new 8.0 release.

I think I'm mildly in favor of getting this done before the 8.0 release. What do you think?

qstokkink commented 3 weeks ago

[Offline discussion summary] Do we even want everything in the same database? Aesthetically, one database file is certainly more appealing. However, from a development perspective, everything is worse: we have to create and maintain complex database migrations and one Tribler experimental module can corrupt the database for all other modules. Current sentiment is to just keep separate database files.

egbertbouman commented 3 weeks ago

@qstokkink Agreed, let's close this.