Grpc Endpoint Caching #42
Conversation
|
|
||
| // Expect Tx error b/c unit test has no network connectivity; do string matching b/c exact error type matching is messy | ||
| assert_eq!(&err[..35], "chain client error: transport error"); |
There was a problem hiding this comment.
Had to clean up these tests here; unrelated to caching PR, they were just in a broken state, error messages aren't always consistent, hence the change.
ocular/src/chain/client/cache.rs
Outdated
| } | ||
| #[derive(Debug, Default, Serialize, Deserialize)] | ||
| pub struct GrpcEndpoint<'a> { | ||
| pub address: &'a str, |
There was a problem hiding this comment.
Is there a particular reason we need a string slice instead of just using String?
There was a problem hiding this comment.
Not that I recall, we can change it if there's a preference
There was a problem hiding this comment.
If we are having to specify a lifetime because we want to make a slice last a long time, that's part of the reason a String exists, so let's do that.
ocular/src/chain/client/cache.rs
Outdated
| override_if_exists: bool, | ||
| ) -> Result<Cache, CacheError> { | ||
| // If none, create at default: (e.g. ~/.ocular/grpc_endpoints.toml) | ||
| let path: String = if let Some(file_path) = file_path { |
There was a problem hiding this comment.
Nit: match is more appropriate and readable here imo, and we also use it elsewhere in the codebase in these situations
There was a problem hiding this comment.
sure, will update.
ocular/src/chain/client/cache.rs
Outdated
| dirs::home_dir() | ||
| .unwrap() | ||
| .into_os_string() | ||
| .into_string() | ||
| .unwrap() | ||
| + DEFAULT_FILE_CACHE_DIR | ||
| + "/" | ||
| + DEFAULT_FILE_CACHE_NAME |
There was a problem hiding this comment.
would probably be safer to use the PathBuf methods (home_dir().unwrap() returns a PathBuf) to build this path. Like push("/" + ...) or something:
https://doc.rust-lang.org/nightly/std/path/struct.PathBuf.html
There was a problem hiding this comment.
Sure, I'll update that.
ocular/src/chain/client/cache.rs
Outdated
| if !path.ends_with(".toml") { | ||
| return Err(CacheError::Initialization(String::from( | ||
| "Only toml files supported.", | ||
| ))); |
There was a problem hiding this comment.
Maybe also check that it is a file and not a directory
ocular/src/chain/client/cache.rs
Outdated
| if !self.endpoints.remove(&item) { | ||
| return Err(CacheError::DoesNotExist(item)); | ||
| } |
There was a problem hiding this comment.
I don't think we need an error for this case since the goal is to make sure it's not in the cache and it already isn't. A debug log might be good indicating that it was attempted but didn't exist?
There was a problem hiding this comment.
Sure, I'm open to that. I think as long as we agree that this isn't actually a failure mode we can omit DNE and already exist cache errors. In theory trying to remove something that isn't present isn't logically correct, but it also might not really matter. What do you think the better user experience would be?
There was a problem hiding this comment.
Yeah I think since it's a cache we don't care about the bool returned by the HashSet for remove(). I don't think the user will care that it didn't exist already.
ocular/src/chain/client/cache.rs
Outdated
| for endpt in &old_toml.endpoint { | ||
| if endpt.address.trim() != item.as_str() { | ||
| toml.endpoint.push(GrpcEndpoint { | ||
| address: endpt.address, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: I think something like this will also
| for endpt in &old_toml.endpoint { | |
| if endpt.address.trim() != item.as_str() { | |
| toml.endpoint.push(GrpcEndpoint { | |
| address: endpt.address, | |
| }); | |
| } | |
| } | |
| toml.endpoint = old_toml.endpoint | |
| .iter() | |
| .filter(|ep| ep.address.trim() != item.as_str()) | |
| .collect(); |
| fn get_all_items(&self) -> Result<HashSet<String>, CacheError> { | ||
| Ok(self.endpoints.clone()) | ||
| } |
There was a problem hiding this comment.
Thinking about how this fits into the health check: is the idea that we would randomly select one of these and then remove the unhealthy ones as we go?
There was a problem hiding this comment.
Yup, that's what I was thinking.
ocular/src/chain/client/cache.rs
Outdated
| #[derive(Debug, Default, Serialize, Deserialize)] | ||
| pub struct GrpcEndpointToml<'a> { | ||
| #[serde(borrow)] | ||
| pub endpoint: Vec<GrpcEndpoint<'a>>, |
There was a problem hiding this comment.
I believe you intended for this to be "endpoints" (plural).
ocular/src/chain/client/cache.rs
Outdated
| fn add_item(&mut self, item: String) -> Result<(), CacheError> { | ||
| match self.endpoints.insert(item.clone()) { | ||
| true => Ok(()), | ||
| false => Err(CacheError::AlreadyExists(item)), | ||
| } | ||
| } | ||
|
|
||
| fn remove_item(&mut self, item: String) -> Result<(), CacheError> { | ||
| match self.endpoints.remove(&item.clone()) { | ||
| true => Ok(()), | ||
| false => Err(CacheError::DoesNotExist(item)), | ||
| } | ||
| } |
There was a problem hiding this comment.
Once again, i think we can ignore the bool returned here.
philipjames44
force-pushed
the
phil/grpc-endpt-optimizations
branch
from
64fb85e to
2246d64
Compare
ocular/src/chain/client/cache.rs
Outdated
| dbg!(format!("Attempting to use path {:#?}", path)); | ||
|
|
||
| // Verify path formatting | ||
| if path.extension().unwrap().to_str().unwrap() != "toml" { |
There was a problem hiding this comment.
This will panic if the path they provide has no file extension.
- probably want a
path.extension().is_none()check that returns an error before this if true - we can avoid the extra unwrap by making "toml" into an
OsString
This covers both of those:
| if path.extension().unwrap().to_str().unwrap() != "toml" { | |
| if path.extension().is_some() && path.extension().unwrap() != OsString::new("toml") { |
Then changing the error message on 74 to something like "Only files with extension .toml are supported"?
ocular/src/chain/client/cache.rs
Outdated
| return Err(CacheError::Initialization(String::from( | ||
| "Only toml files supported.", | ||
| ))); | ||
| } else if path.is_dir() { |
There was a problem hiding this comment.
Checking this first would avoid the no-extension error
There was a problem hiding this comment.
Yup, reordered
ocular/src/chain/client/cache.rs
Outdated
| let save_path = path.parent().unwrap(); | ||
|
|
||
| let dir_res = save_path.metadata(); | ||
|
|
||
| // Create dir if doesn't exist | ||
| if dir_res.is_err() { |
There was a problem hiding this comment.
Path has an exists() method
| let save_path = path.parent().unwrap(); | |
| let dir_res = save_path.metadata(); | |
| // Create dir if doesn't exist | |
| if dir_res.is_err() { | |
| let save_path = path.parent().unwrap(); | |
| // Create dir if doesn't exist | |
| if !save_path.exists() { |
ocular/src/chain/client/cache.rs
Outdated
| let mut endpoints = HashSet::new(); | ||
|
|
||
| // Load endpoints if they exist | ||
| if std::path::Path::new(&path).exists() { |
There was a problem hiding this comment.
pretty sure this can be path.exists()?
ocular/src/chain/client/cache.rs
Outdated
| } | ||
|
|
||
| // Finally we can manipulate the actual file after checking the override settings | ||
| if override_if_exists || !std::path::Path::new(&path).exists() { |
ocular/src/chain/client/cache.rs
Outdated
| } | ||
| } | ||
|
|
||
| // If patch specified create |
There was a problem hiding this comment.
yup, removed.
ocular/src/chain/client/cache.rs
Outdated
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
These tests seem involved enough to warrant being in the integration tests dir and run in a docker env since they write to the file system
There was a problem hiding this comment.
moved to docker integration tests
This PR adds in a grpc endpoint cache that adds:
1.) Option to read and write from file based cache
2.) Option for in ephemeral in-memory caching
3.) Option to have no caching whatsoever
This PR should be followed by #23 to actually utilize the cache. This is split into 2 issues to separate actual cache implementation and utilization bc the latter will require a bit of a refractor.