-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: add cache in cac client #268
base: main
Are you sure you want to change the base?
Conversation
863d2ac
to
7487820
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FFI client side changes review pending
} | ||
Ok(cac) | ||
let filter_string = if let Some(mut vec) = filter_keys.clone() { | ||
vec.sort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make sure this is a unique list ?
maybe something like, Vec => HashSet => String
clients/go/cacclient/main.go
Outdated
if tenantName == "" { | ||
return nil, errors.New("tenantName cannot be Empty") | ||
} | ||
if cacHostName == "" { | ||
return nil, errors.New("cacHostName cannot be Empty") | ||
} | ||
if cacheMaxCapacity != nil { | ||
cacheCapacity = &(C.ulong(*cacheMaxCapacity)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does go not require declaration of the variable first ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
crates/cac_client/src/interface.rs
Outdated
@@ -102,15 +102,40 @@ pub extern "C" fn cac_new_client( | |||
tenant: *const c_char, | |||
update_frequency: c_ulong, | |||
hostname: *const c_char, | |||
cache_max_capacity: *const c_char, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this defined as pointer to c_char
, this should be same as others *const c_ulong
crates/cac_client/src/lib.rs
Outdated
merge_strategy, | ||
) | ||
let context_key = json_to_sorted_string(&json!(query_data)); | ||
let value = self.config_cache.get(&context_key).unwrap_or({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let value = self.config_cache.get(&context_key).unwrap_or({ | |
let value = self.config_cache.get(&context_key).unwrap_or_else(|| { |
crates/cac_client/src/lib.rs
Outdated
&cac.overrides, | ||
&query_data, | ||
merge_strategy, | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we cache the error as well to avoid evaluation when we knows it going to fail anyways ?
crates/cac_client/src/lib.rs
Outdated
&cac.overrides, | ||
&query_data, | ||
merge_strategy, | ||
)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap_or_else
would not work if you want to use early return from here
in that case you would have to convert this to a match
block
crates/cac_client/src/lib.rs
Outdated
&query_data, | ||
merge_strategy, | ||
) | ||
let context_key = json_to_sorted_string(&json!(query_data)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let context_key = json_to_sorted_string(&json!(query_data)); | |
let context_key = json_to_sorted_string(&Value::Object(query_data)); |
crates/cac_client/src/lib.rs
Outdated
} else { | ||
"null".to_string() | ||
}; | ||
let hash_key = json_to_sorted_string(&json!(query_data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let hash_key = json_to_sorted_string(&json!(query_data)) | |
let hash_key = json_to_sorted_string(&Value::Object(query_data)) |
crates/cac_client/src/interface.rs
Outdated
) -> c_int { | ||
let duration = Duration::new(update_frequency, 0); | ||
let tenant = unwrap_safe!(cstring_to_rstring(tenant), return 1); | ||
let hostname = unwrap_safe!(cstring_to_rstring(hostname), return 1); | ||
|
||
let cache_max_capacity = if !cache_max_capacity.is_null() { | ||
Some(unsafe { *cache_max_capacity as u64 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, lets add a function maybe clong_to_ru64
just like cstring_to_rstring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FFI client side changes review
@@ -80,12 +81,22 @@ getError = c_last_error_message | |||
cleanup :: [Ptr a] -> IO () | |||
cleanup items = mapM free items $> () | |||
|
|||
createCacClient:: Tenant -> Integer -> String -> IO (Either Error ()) | |||
createCacClient tenant frequency hostname = do | |||
allocateAndPoke :: Maybe Integer -> IO (Ptr CULong) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we name it something like
allocateAndPoke :: Maybe Integer -> IO (Ptr CULong) | |
allocateCLongPtr :: Maybe Integer -> IO (Ptr CULong) |
although its using poke
internally, having poke
in name, made it really confusing to understand what was going on here
002fe5b
to
f5ce888
Compare
crates/cac_client/Cargo.toml
Outdated
@@ -19,6 +19,8 @@ serde_json = { workspace = true } | |||
strum = { workspace = true } | |||
strum_macros = { workspace = true } | |||
tokio = { version = "1.29.1", features = ["full"] } | |||
mini-moka = { workspace = true } | |||
itertools = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there seems to be a misunderstanding, I was asking to move only itertools
to workspace as that is the only only common one, and let mini-moka
be defined here only
also, update the Cargo.toml
in context-aware-config
to make the itertools
over there also pick from workspace
also, can you please make sure that the entries in all the 3 modified toml files are alphabetically sorted
f5ce888
to
208c996
Compare
@ayushjain17 I have used go formatter , it does not formats that function part |
f526b62
to
5003128
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few points of verification and earlier missed out errors
clients/java/cac-client/src/main/java/cac_client/CacClient.java
Outdated
Show resolved
Hide resolved
@@ -1,7 +1,7 @@ | |||
package expclient | |||
|
|||
/* | |||
#include "libexperimentation_client.h" | |||
#include "../../../headers/libexperimentation_client.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Datron can you please verify the path change for the header file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pratikmishra356 don't change this, this go file runs in the users system, not inside superposition repo
@@ -1,7 +1,7 @@ | |||
package cacclient | |||
|
|||
/* | |||
#include "libcac_client.h" | |||
#include "../../../headers/libcac_client.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Datron can you please verify the path change for the header file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pratikmishra356 we shouldn't change this, rather use $SUPERPOSITON_INCLUDE_PATH/libcac_client.h
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont we need to update the path for the header files for Haskell client ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont we need to update the path for the header files for Java client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so
5003128
to
d617c98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verification of path changes for header files is pending across the different FFI clients, otherwise its fine
clients/go/main.go
Outdated
@@ -2,6 +2,7 @@ package main | |||
|
|||
import ( | |||
"fmt" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this newline between imports
d617c98
to
31052cb
Compare
@@ -1,7 +1,7 @@ | |||
package cacclient | |||
|
|||
/* | |||
#include "libcac_client.h" | |||
#include "../../../headers/libcac_client.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pratikmishra356 we shouldn't change this, rather use $SUPERPOSITON_INCLUDE_PATH/libcac_client.h
@@ -1,7 +1,7 @@ | |||
package expclient | |||
|
|||
/* | |||
#include "libexperimentation_client.h" | |||
#include "../../../headers/libexperimentation_client.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pratikmishra356 don't change this, this go file runs in the users system, not inside superposition repo
if error != nil { | ||
fmt.Println(error) | ||
} | ||
|
||
fmt.Println("\n------------Configs----------------------------\n") | ||
fmt.Println("\n------------Configs----------------------------") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why remove \n
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as it is already using println , that's why
cacheCapacity <- allocateCLongPtr cacheMaxCapacity | ||
cacheTimeToLive <- allocateCLongPtr cacheTTL | ||
cacheTimeToIdle <- allocateCLongPtr cacheTTI | ||
resp <- c_new_cac_client cTenant duration cHostname cacheCapacity cacheTimeToLive cacheTimeToIdle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: Align the arrows for style
@@ -108,7 +119,7 @@ getFullConfigStateWithFilter client mbFilters mbPrefix = do | |||
cPrefix <- case mbPrefix of | |||
Just prefix -> newCAString (intercalate "," prefix) | |||
Nothing -> return nullPtr | |||
config <- withForeignPtr client $ \client -> c_get_config client cFilters cPrefix | |||
config <- withForeignPtr client $ \val -> c_get_config val cFilters cPrefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its client pointer, why rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Datron it was giving error , as we are using client name in functional parameter also
@@ -132,7 +143,7 @@ getResolvedConfigWithStrategy client context mbKeys mergeStrat = do | |||
cStrKeys <- case mbKeys of | |||
Just keys -> newCAString (intercalate "|" keys) | |||
Nothing -> return nullPtr | |||
overrides <- withForeignPtr client $ \client -> c_cac_get_resolved_config client cContext cStrKeys cMergeStrat | |||
overrides <- withForeignPtr client $ \val -> c_cac_get_resolved_config val cContext cStrKeys cMergeStrat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overrides <- withForeignPtr client $ \val -> c_cac_get_resolved_config val cContext cStrKeys cMergeStrat | |
overrides <- withForeignPtr client $ \client -> c_cac_get_resolved_config client cContext cStrKeys cMergeStrat |
@@ -11,7 +11,7 @@ import Prelude | |||
|
|||
main :: IO () | |||
main = do | |||
createCacClient "dev" 10 "http://localhost:8080" >>= \case | |||
createCacClient "dev" 10 "http://localhost:8080" Nothing Nothing Nothing>>= \case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createCacClient "dev" 10 "http://localhost:8080" Nothing Nothing Nothing>>= \case | |
createCacClient "dev" 10 "http://localhost:8080" Nothing Nothing Nothing >>= \case |
config = config.filter_by_prefix(&HashSet::from_iter(keys)); | ||
let filter_string = if let Some(vec) = filter_keys.clone() { | ||
let hset: HashSet<String> = HashSet::from_iter(vec); | ||
let mut vec: Vec<String> = hset.iter().cloned().collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need a better variable name
cache_max_capacity: *const c_ulong, | ||
cache_ttl: *const c_ulong, | ||
cache_tti: *const c_ulong, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pratikmishra356 instead of taking a pointer so you call make it null, would it be better to take long itself? if the value is 0, you can pass None to the client create, else Some(long). This simplifies your FFI implementation as well and you don't have to deal with null pointers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also prefer not using null pointer, but won't this make the contract inconsistent, if someone is using a Rust client they can give None or Some but for other client they have to give an int or long everytime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I too wanted to avoid the pointer,
but with 0 if we are falling back to default, we are forcing everyone to use the caching no matter
if someone, for some reasons want to have no caching he could have send 0 as the value, but then with this check the FFI clients wont be able to do this, leading to inconsistency
let hset: HashSet<String> = HashSet::from_iter(vec); | ||
let mut vec: Vec<String> = hset.iter().cloned().collect(); | ||
vec.sort(); | ||
vec.join(",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let hset: HashSet<String> = HashSet::from_iter(vec); | |
let mut vec: Vec<String> = hset.iter().cloned().collect(); | |
vec.sort(); | |
vec.join(",") | |
BTreeSet::from_iter(vec).iter().join(",") |
This can be written as such, create a HashSet first and then sorting seems like extra operation.
cache_max_capacity: *const c_ulong, | ||
cache_ttl: *const c_ulong, | ||
cache_tti: *const c_ulong, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also prefer not using null pointer, but won't this make the contract inconsistent, if someone is using a Rust client they can give None or Some but for other client they have to give an int or long everytime.
Problem
Add cache functionality in client eval
Solution
Used mini-moka , and added new optional parameters while creating new client
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change