@antirez there are a few things that came up and we'd like to run by you. Maybe we'll open separate PRs or issues for them once you give some initial feedback.

  1. ~~There are no module APIs for RESP3, e.g. addReplyArrayLen vs addReplyMapLen~~
  2. ~~There seem to be nowhere that documents the meaning of each ACL category (in the code), this means that we might one day interpret @admin as having one purpose, and the next day as something else. for instance, does @read means anything that's not @write what about commands that don't read from the keyspace, should they have @read or not?~~
  3. ~~i'm not sure that TIME, LASTSAVE, ECHO, ROLE, should have the read-only flag (they don't access the keyspace)~~
  4. ~~I think you should go over the module APIs that are marked as experimental, and move many of them outside of that block (no longer experimental).~~
  5. ~~srandmemberWithCountCommand sometimes calls the old dictGetRandomKey (not the fair one), is that on purpose, or was overlooked?~~
  6. when config.c enables AOF, it immediately starts an AOFRW. maybe we want to have a similar thing when the save config is changed? i.e when it changes from nothing to something, trigger a bgsave implicitly?
  7. ~~don't GEORADIUS and GEORADIUSBYMEMBER need use-memory flag? (like SORT does)~~
  8. ~~recently in #6401 we fixed a bug with WATCH and -OOM, i now realize that the same bug will happen with other errors that return from processCommand, such as -MISCONF~~
  9. ~~We noticed that expire.c calls trackingInvalidateKey, why doesn't evict.c do the same? also don't both of these need to call signalModifiedKey? (to invalidate watch too)~~
  10. An idea came up to consider changing addReplyDouble (or add addReplyLongDouble) to use "%La" format, so that it doesn't lose precision (like we did for RM_SaveLongDouble).
  11. ~~In the past we discussed a module API named RM_KeyExist, if we want to add one, should it be based on lookupKey (which does implicit expiry attempt) or dbExists (which doesn't, in which case a follow up attempt to open the key may fail)~~
  12. ~~I think we may want to add module hooks for fork. i.e. like atfork, one before the fork in the parent process, and two after the fork in each process. i suppose that these should be called with the GIL unlocked, so that modules with threads doing jobs can drain their job queue before the split happens.~~
  13. ~~similarly I think we may want a module hook before redis iterates on the keyspace (e.g. foreground save, debug digest, etc), so that modules with threads can drain their job queue.~~

If you can please provide a short response for each telling me how you'd like to proceed with it, fixing yourself, rejecting the topic, or asking that we'll open a PR or a separate issue.

Thanks.

Comment From: madolson

Did we do all these things?

Comment From: madolson

@oranagra ^

Comment From: oranagra

@madolson we didn't do anything from that list except for number 8 (now marked as strike though) and i don't think i even noticed that Salvatore gave me a thumbs up for my slack post about the topic of #7918. we need to pick up on this list and handle most of them.

Comment From: ShooterIT

For 6, I am not sure, actually, we will start bgsave in serverCron if server reaches the given amount of changes during the given amount of seconds.

Comment From: oranagra

but i think what i was thinking is that we might wanna trigger a BGSAVE right away even if the save criteria (server.dirty counter) aren't met yet. i.e. in the case that before the CONFIG SET the server wasn't set to persist RDB at all, and after it, it has some RDB persistence configuration, and there's absolutely no RDB persistence on disk, we may want to create one ASAP.

Comment From: ShooterIT

I think save is not similar to appendonly, appendonly only has yes or no options, and we append new commands to aof if appendonly is yes. Our current usage for save, it starts bgsave only if satisfy some conditions, and we like to use bgsave to create RDB if we want one immediately.

Comment From: ShooterIT

For item3, I think we may should remove the read-only flag For item12, multi-thread is not friendly with fork, i think it is necessary in future. I remembered localtime(called in serverLog) results child process deadlock.

Comment From: oranagra

taking care of item 3 in #8216 @MeirShpilraien will take item 12 soon.

Comment From: MeirShpilraien

After discussion with @oranagra about 12 we think it might be dangerous and we would like to suggest another approach and hear your opinions.

The reason it's dangerous is that when a module calls RM_Fork it assumes that the fork process is created atomically (because the GIL is taken) and the database state of the fork will be the same as the parent when the fork was initiated. If we introduce these atfork callbacks which will be called when the GIL is released we will break this assumption (notice that for rdb save, this assumption is not critical, but we do not know what modules will do with RM_Fork and for modules that uses this API it might be critical...)

We suggest another approach, a module can use the pthread_atfork callbacks as is and if the GIL needs to be released (for example to drain some threads that need to finish some jobs) then the prepare callback can release the GIL (RM_ThreadSafeCtxUnlock) and re-acquire it (RM_ThreadSafeCtxLock) before return. This is of course under the module responsibility assuming that the module knows what he is doing and it's OK to that.

Let us know what you think.

Comment From: enjoy-binbin

For item 5. I think it was simply overlooked. I think it should use the fair one. 5. srandmemberWithCountCommand sometimes calls the old dictGetRandomKey (not the fair one), is that on purpose, or was overlooked?

Comment From: enjoy-binbin

I saw oran reacted with a thumbs up. I also found hrandfieldWithCountCommand and zrandmemberWithCountCommand both used the dictGetRandomKey, not the fair one. Add here to prevent forgetting. (Thoes code was copied from set)

Comment From: oranagra

Module API for RESP3 handled here https://github.com/redis/redis/pull/8521

Comment From: hwware

Hey, I want to proceed with number 11 (RM_KeyExists), but first can we discuss the requirements for how we want to implement the API? @oranagra

Comment From: oranagra

@hwware I'm not sure we need that API, dbExist was already deleted, and in any way I think it should probably use lookupKey (and do implicit expiry attempt), in which case maybe the current RM_OpenKey is enough. @guybe7 @MeirShpilraien any other thoughts?

Comment From: hwware

I think that if you use RM_OpenKey then you would also need to use RM_CloseKey to close the handle for that string. So RM_KeyExists would avoid that. Is that a valid reason to implement this?

Comment From: oranagra

i'm not sure it's a justification, the overhead is not high, i don't have a clear opinion on that matter.. at the time i was thinking maybe it'll do something that's conceptually different so i came with this question.

Comment From: oranagra

number 5 handled in #9538

Comment From: enjoy-binbin

when config.c enables AOF, it immediately starts an AOFRW. maybe we want to have a similar thing when the save config is changed? i.e when it changes from nothing to something, trigger a bgsave implicitly?

i think it is a good idea. i am guessing the easiest way is, in setConfigSaveOption

setConfigSaveOption {
    int before_save_enabled = server.saveparamslen != 0;

    old code

    int after_save_enabled = server.saveparamslen != 0;

    if (!before_save_enabled && after_save_enabled) {
        /* start a bgsave if needed */
        server.rdb_bgsave_scheduled = 1;
    }

    return 1;
}

Comment From: oranagra

I remember discussing this with Salvatore and he argued against it, but i no longer remember why. p.s. there's also #6217 which is somewhat in that area.