Host list #19
Host list #19
Conversation
This commit adds a minimal build of a proxysql, mysql and readyset via docker compose file. With this we can start a local environment to test the scheduler.
This commit adds the ability to read a list of hosts from mysql_servers table instead of a single host from config file. This merges the old server.rs into the hosts file. This also remove the Pooled connection to a single connection. Fixes: #5
| Online, | ||
| //backend sever is temporarily taken out of use because of either too many connection errors in a time that was too short, or the replication lag exceeded the allowed threshold | ||
| Shunned, | ||
| //when a server is put into OFFLINE_SOFT mode, no new connections are created toward that server, while the existing connections are kept until they are returned to the connection pool or destructed. In other words, connections are kept in use until multiplexing is enabled again, for example when a transaction is completed. This makes it possible to gracefully detach a backend as long as multiplexing is efficient |
There was a problem hiding this comment.
Seems like you might want to run cargo fmt to wrap these longer lines
There was a problem hiding this comment.
Should the rest of these be /// comments too for doc comment purposes?
There was a problem hiding this comment.
The longer lines do not format. Currently this project enforces cargo fmt already.
I can see we use a specific config to wrap comments on Readyset, however, those features are from nightly:
marcelo Work/readyset_proxysql_scheduler [query-search-method] $ cargo fmt --check
Warning: can't set `wrap_comments = true`, unstable features are only available in nightly channel.
Warning: can't set `format_code_in_doc_comments = true`, unstable features are only available in nightly channel.
Warning: can't set `comment_width = 100`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Module`, unstable features are only available in nightly channel.
Warning: can't set `group_imports = StdExternalCrate`, unstable features are only available in nightly channel.
Added the /// to make it consistent.
| None => Ok(false), | ||
| } | ||
| } | ||
| None => Ok(false), |
There was a problem hiding this comment.
I don't know one example of when EXPLAIN will return no rows. I am running the query with ? which will make the caller return the same error. I think it still make sense to differ an Error from an empty result.
| /// | ||
| /// # Returns | ||
| /// | ||
| /// true if the query was cached successfully, false otherwise. |
There was a problem hiding this comment.
Might make sense to return a descriptive error if there's no connection instead of false
There was a problem hiding this comment.
Make sense. Just FYI, this part is unreachable. The only reason for having conn as an Option is to allow the creation of a Host even if a connection fail. Later at ProxySQL::health_check we call Host::check_readyset_is_ready if conn is None we return an error and mark the node as Shunned. Cache a query comes as a later point after where we only kept the online hosts.
Fixed
| None => return Ok(false), | ||
| Some(conn) => { | ||
| conn.query_drop(format!("CREATE CACHE d_{} FROM {}", digest, digest_text)) | ||
| .expect("Failed to create readyset cache"); |
There was a problem hiding this comment.
I think this could fail for a lot of unexpected reasons and we should probably return an error instead of panic.
There was a problem hiding this comment.
Fixed. Btw, we are panicking in the caller if cache_query returns an error. Basically we are iterating through all online readyset hosts and if one readyset fails to cache a query, this is one unrecoverable error. Given the nature of the scheduler (crontab that runs every X seconds) I don't see any difference in panicking right away versus handling an error.
| ps_conn: &mut Conn, | ||
| config: &Config, | ||
| status: HostStatus, | ||
| ) -> Result<bool, mysql::Error> { |
There was a problem hiding this comment.
Doesn't look like we return anything other than true so we should probably return Result<(), ...>
There was a problem hiding this comment.
This function migrated to ProxySQL::health_check - ProxySQL handles all the changes required to proxysql and host.change_status is just an in memory version of the state of the host.
| Online, | ||
| //backend sever is temporarily taken out of use because of either too many connection errors in a time that was too short, or the replication lag exceeded the allowed threshold | ||
| Shunned, | ||
| //when a server is put into OFFLINE_SOFT mode, no new connections are created toward that server, while the existing connections are kept until they are returned to the connection pool or destructed. In other words, connections are kept in use until multiplexing is enabled again, for example when a transaction is completed. This makes it possible to gracefully detach a backend as long as multiplexing is efficient |
There was a problem hiding this comment.
Should the rest of these be /// comments too for doc comment purposes?
| /// Gets the hostname of the host. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// The hostname of the host. |
There was a problem hiding this comment.
nit: This is a lot of vertical space for a comment for an accessor function
There was a problem hiding this comment.
I have used the same comment formatting for all functions. Do you want to remove the empty lines ?
There was a problem hiding this comment.
No, I don't think I'm asking for you to do that, and if you have a consistent thing going on, I don't care that much that it be changed.
I think the value of comments for accessor functions is pretty low, as this tells the reader the same thing (get the hostname) 3 times (including the name of the function). If you can comfortably make these types of comments just 1 line, I do think that would be an improvement.
Feel free to ignore this advice.
|
closing in favor of #21 |
This commit adds the ability to read a list of hosts from mysql_servers
table instead of a single host from config file.
This merges the old server.rs into the hosts file.
This also remove the Pooled connection to a single connection.
Fixes: #5