Skip to content
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

Is there an API for RedisKeyWritable to remove expiration for a key #275

Open
QuChen88 opened this issue Feb 3, 2023 · 2 comments
Open

Comments

@QuChen88
Copy link
Contributor

QuChen88 commented Feb 3, 2023

I noticed that when I open a key for writing via context.open_key_writable(), I can only call set_expire() with a duration that is >= 0. However, I noticed the low level API raw::set_expire() takes the expiration timestamp as c_long_long which can be REDISMODULE_NO_EXPIRE to make the key not expire. In this case there doesn't seem to be a way for me to do this using the set_expire() API. See

pub fn set_expire(&self, expire: Duration) -> RedisResult {

i.e.

let key = ctx.open_key_writable(key);
match key.set_expire(Duration::new(5, 0)) {
     Ok(v) => Ok(v),
     Err(e) => Err(e)
}

Is there a way for me to remove the expiration of the key using the RedisKeyWritable APIs? I tried to call the raw::set_expire() method but it seems like it needs RedisModuleKey as parameter. But the RedisModuleKey field inside RedisKeyWritable is private and can't be accessed. See

key_inner: *mut raw::RedisModuleKey,

@QuChen88
Copy link
Contributor Author

QuChen88 commented Feb 4, 2023

Submitted PR to address this #276

@QuChen88 QuChen88 changed the title Is there a way to call set_expire() with REDISMODULE_NO_EXPIRE to persist the key Is there an API for RedisKeyWritable to remove expiration for a key Feb 4, 2023
@oshadmi
Copy link
Contributor

oshadmi commented Feb 4, 2023

@gkorland @MeirShpilraien Another option instead of adding a remove_expire method (in PR #276) is to change the signature of set_expire to receive a c_longlong instead of Duration, so the value REDISMODULE_NO_EXPIRE could be passed as well, which will allow to remove expiration.
Currently Duration is unsigned, and when receiving values beyond i64, i64::try_from will fail.
Changing to c_longlong would fail in build time instead of run time with values beyond i64.
However since changing the API (to be stricter) is a breaking change it can only be done in a major version, so we should also add a new remove_expire method.
Agree?

BTW, this would not entirely prevent failures even with c_longlong value, but would narrow down the current range of erroneous values (i64::try_from would still be able to fail since value is multiplied by 1000 when converting to nanoseconds)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants