-
Notifications
You must be signed in to change notification settings - Fork 41
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
Rename tracker
mod to core
#255
Comments
Sounds reasonable to me :) |
Hi @da2ce7, I've been giving more thought to this. These are the packages we have extracted so far: packages/
├── configuration
├── located-error
├── primitives
└── test-helpers In the long-term I see we could have the following: packages/
├── api <- NEW!: torrust-tracker-api (servers)
├── udp <- NEW!: torrust-tracker-udp (servers)
├── http <- NEW!: torrust-tracker-http (servers)
├── core <- NEW!: torrust-tracker-core (tracker)
├── bittorrent <- NEW!: torrust-tracker-bittorrent (shared)
├── clock <- NEW!: torrust-tracker-clock (shared)
├── crypto <- NEW!: torrust-tracker-crypto (shared)
├── configuration <- torrust-tracker-configuration
├── located-error <- torrust-tracker-located-error
├── primitives <- torrust-tracker-primitives
└── test-helpers <- torrust-tracker-test-helpers The "shared" ones could be a single package, but as they do not have much in common and could be used independently, I would keep them in separate packages. I would also refactor the config file from: log_level = "debug"
mode = "public"
db_driver = "Sqlite3"
db_path = "./storage/database/data.db"
announce_interval = 120
min_announce_interval = 120
max_peer_timeout = 900
on_reverse_proxy = false
external_ip = "2.137.87.41"
tracker_usage_statistics = true
persistent_torrent_completed_stat = true
inactive_peer_cleanup_interval = 600
remove_peerless_torrents = false
[[udp_trackers]]
enabled = true
bind_address = "0.0.0.0:6969"
[[http_trackers]]
enabled = true
bind_address = "0.0.0.0:7070"
ssl_enabled = false
ssl_cert_path = "./storage/ssl_certificates/localhost.crt"
ssl_key_path = "./storage/ssl_certificates/localhost.key"
[http_api]
enabled = true
bind_address = "0.0.0.0:1212"
ssl_enabled = false
ssl_cert_path = "./storage/ssl_certificates/localhost.crt"
ssl_key_path = "./storage/ssl_certificates/localhost.key"
[http_api.access_tokens]
admin = "MyAccessToken" to: log_level = "debug"
[core_tracker]
mode = "public"
db_driver = "Sqlite3"
db_path = "./storage/database/data.db"
announce_interval = 120
min_announce_interval = 120
max_peer_timeout = 900
on_reverse_proxy = false
external_ip = "2.137.87.41"
tracker_usage_statistics = true
persistent_torrent_completed_stat = true
inactive_peer_cleanup_interval = 600
remove_peerless_torrents = false
[[udp_trackers]]
enabled = true
bind_address = "0.0.0.0:6969"
[[http_trackers]]
enabled = true
bind_address = "0.0.0.0:7070"
ssl_enabled = false
ssl_cert_path = "./storage/ssl_certificates/localhost.crt"
ssl_key_path = "./storage/ssl_certificates/localhost.key"
[http_api]
enabled = true
bind_address = "0.0.0.0:1212"
ssl_enabled = false
ssl_cert_path = "./storage/ssl_certificates/localhost.crt"
ssl_key_path = "./storage/ssl_certificates/localhost.key"
[http_api.access_tokens]
admin = "MyAccessToken" The configuration will also change accordingly from: pub struct Configuration {
pub log_level: Option<String>,
pub mode: TrackerMode,
pub db_driver: DatabaseDriver,
pub db_path: String,
pub announce_interval: u32,
pub min_announce_interval: u32,
pub max_peer_timeout: u32,
pub on_reverse_proxy: bool,
pub external_ip: Option<String>,
pub tracker_usage_statistics: bool,
pub persistent_torrent_completed_stat: bool,
pub inactive_peer_cleanup_interval: u64,
pub remove_peerless_torrents: bool,
pub udp_trackers: Vec<UdpTracker>,
pub http_trackers: Vec<HttpTracker>,
pub http_api: HttpApi,
} to: pub struct Configuration {
pub log_level: Option<String>,
pub core_tracker: CoreTracker,
pub udp_trackers: Vec<UdpTracker>,
pub http_trackers: Vec<HttpTracker>,
pub http_api: HttpApi,
}
pub struct CoreTracker {
pub mode: TrackerMode,
pub db_driver: DatabaseDriver,
pub db_path: String,
pub announce_interval: u32,
pub min_announce_interval: u32,
pub max_peer_timeout: u32,
pub on_reverse_proxy: bool,
pub external_ip: Option<String>,
pub tracker_usage_statistics: bool,
pub persistent_torrent_completed_stat: bool,
pub inactive_peer_cleanup_interval: u64,
pub remove_peerless_torrents: bool,
} I'm tempted to do those refactorings before continuing with the crate's documentation. I would also move packages/
├── api <- NEW!: torrust-tracker-api (servers)
├── udp <- NEW!: torrust-tracker-udp (servers)
├── http <- NEW!: torrust-tracker-http (servers)
├── api-config <- NEW!: torrust-tracker-api-config
├── udp-config <- NEW!: torrust-tracker-udp-config
├── http-config <- NEW!: torrust-tracker-http-config
├── core-config <- NEW!: torrust-tracker-core-config
... I would keep the current pub struct Configuration {
pub log_level: Option<String>,
pub core_tracker: CoreTracker,
pub udp_trackers: Vec<UdpTracker>,
pub http_trackers: Vec<HttpTracker>,
pub http_api: HttpApi,
} What do you think @da2ce7 @WarmBeer? We can do it later, but it will cost extra effort to refactor the documentation. Regarding the config file, that would be a big change that would need to wait until the next major release (V4) if we want to do it. |
And maybe we should rename some of the packages I've mentioned avobe: packages/
├── http-api <- NEW!: torrust-tracker-http-api (renamed)
├── udp-tracker <- NEW!: torrust-tracker-udp-tracker (renamed)
├── http-tracker <- NEW!: torrust-tracker-http-tracker (renamed)
├── core-tracker <- NEW!: torrust-tracker-core-tracker (renamed)
├── bittorrent <- NEW!: torrust-tracker-bittorrent
├── clock <- NEW!: torrust-tracker-clock
├── crypto <- NEW!: torrust-tracker-crypto
├── configuration <- torrust-tracker-configuration
├── located-error <- torrust-tracker-located-error
├── primitives <- torrust-tracker-primitives
└── test-helpers <- torrust-tracker-test-helpers I would like the packages to match the log_level = "debug"
[core_tracker]
...
[[udp_trackers]]
...
[[http_trackers]]
...
[http_api]
...
[http_api.access_tokens]
... |
Should we remove the packages/
├── http-api <- torrust-tracker-http-api
├── udp-tracker <- torrust-udp-tracker
├── http-tracker <- torrust-http-tracker
├── core-tracker <- torrust-core-tracker
├── bittorrent <- torrust-bittorrent
├── clock <- torrust-clock
├── crypto <- torrust-crypto
├── configuration <- torrust-tracker-configuration
├── located-error <- torrust-tracker-located-error
├── primitives <- torrust-tracker-primitives
└── test-helpers <- torrust-tracker-test-helpers |
@josecelano https://github.com/greatest-ape/aquatic uses fully-referenced folder names. However I don't think that it is needed necessarily... |
Hi @da2ce7, I'm ok with keeping the folder names shorter (left side) and different from the crate's name. Notice that on the right side, I wrote the cargo crate names. With this reorganization, we have two main advantages:
But as I said, we can postpone it. Until now, the reason to extract a package has always been to resolve cycle dependencies and not to reuse or decouple components. Maybe that could be a goal for version |
I like the idea of changing the name from tracker to core, it might be generic but I think it is effective and self explanatory. Also less redundant, as we are not using the word tracker so many times. I also like keeping the packages's names shorter and keeping the different configuration options separated. Leaving these changes to the next major release is a good idea, as they are important but don´t affect how the app works or it's performance and there are more important feats/bugs/etc to focus on for now. |
@josecelano and @mario-nt I think that it is a simple refactor, should be a couple of hours work at most; It could be done anytime. |
I propose renaming the root module
tracker
tocore
.Mod: https://github.com/torrust/torrust-tracker/tree/develop/src/tracker
That will allow having these namespaces:
core::tracker::Tracker
core::peer::Peer
In my opinion, that's better than the:
tracker::Tracker
tracker::peer::Peer
The mod contains more things than just the tracker, and that
Tracker
could even disappear in the future. I think what we have is the domain logic for BitTorrent trackers, regardless of which delivery mechanism you use (the way end-user interact with that domain logic).I'm writing the documentation for the crate, and it makes sense to give it a name to the "core" tracker logic.
If we move the mod to a package in the future, the name would be
torrust-tracker-tracker
if we follow current conventions. I thinktorrust-tracker-core
would be a better name.I'm open to more options. I think
core
may be too generic.Other alternatives:
torrust-tracker-domain
torrust-bittorrent-tracker
We must consider that we also have another package called
shared
with logic used by thecore
,http
,udp
, ...cc @da2ce7 @WarmBeer
The text was updated successfully, but these errors were encountered: