-
Notifications
You must be signed in to change notification settings - Fork 70
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
rfc: refactoring Server
#640
Comments
Sounds reasonable! I don't even think we need a UUID. Since the server name from the toml config has to be unique, we can simply hash it and use that as the u64 id. |
Yes, but the issue is that since bouncer-networks (#77) would mean multiple servers / connections for a single TOML "server name", we need a better way to identify servers for example, two servers from two different bouncers could be called "libera" |
Makes sense 👍 |
Ok i'll try to work on this separately from the bouncer-networks stuff (since that's already getting kind of complicated) and we'll see how it looks 🎉 |
I think I'm going to try to make it a v5 UUID with a consistent hash function, so that we can use these in the history in lieu of server names |
The Problem
When working on #77 I have run into numerous issues with how to represent a "server" in halloy. The
Server
struct, which is a wrapper aroundString
, is currently ubiquitous in the halloy codebase, however it has many issues. SinceServer
is aString
,&Server
is&String
, which is an anti-pattern. Variables of type&Server
must be.clone()
d often to satisfy lifetime issues and in order to satisfy lifetime constraints, which leads to the same "server name" being duplicated all over the codebase. For example, it must be cloned every time a message is received by the IRC server to communicate the server updated between the "stream" and "client".In order to implement #77, the way that servers are referenced must be changed as well. In a bouncer-networks setup, several "primary" servers each set up any number of secondary servers. Thus there is a hierarchical relationship between servers, and a single config is shared between many. Furthermore, servers can no longer be identified by their name. Names may not be unique (two secondaries on under two different primaries may match), and are not the canonical way to refer to a server (the canonical identifier for a "secondary" server is its bouncer NETID).
Therefore, however we represent
Server
, it cannot also be used as the human-visible name for a server, which causes issues elsewhere in the codebaseA Proposed Solution
Ideally, in order to send a
Server
identifier with every single irc message, theServer
type should beCopy
(not justClone
!) and cheap to replicate. Furthermore, we should be able to get additional data that aServer
represents by asking a centralized source of information.My proposal is as follows:
Server
should be aUuid
(either v4 or v5). This data can be cheaply copied and sent between tasks without a performance overhead. Then, when it is necessary to look up server information (such as user-visible name) this can be gathered by looking at the aHashMap<Uuid, ServerInfo>
which contains server config name, bouncer ID, bouncer name, etc.CCs
@tarkah , @casperstorm let me know what you think. Last time I brought this up you mentioned that I should open an RFC before doing any huge refactors with
Server
and friends 😸The text was updated successfully, but these errors were encountered: