Modules: In RM_HashSet, add COUNT_ALL flag and set errno#8446
Modules: In RM_HashSet, add COUNT_ALL flag and set errno#8446oranagra merged 4 commits intoredis:unstablefrom
Conversation
|
Nice! LGTM |
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
i haven't reviewed the test.
also i didn't read the discussion that lead to this recently, so please let me know if i'm missing some point or going against something that was already agreed.
src/module.c
Outdated
There was a problem hiding this comment.
i think the term "inserted" is confusing ("updated" is confusing too).
"updated" in this case means a pre-existing member was updated.
but i think the term can also mean new field that was added (the hash object was updated).
so i think we should make it clear in the docs.
similarly, "inserted" sounds like it only means new fields (excluding pre-existing fields), but the code actually means any field that is "set" (both new, and updates). let's try to come up with a better term, and if not, at least make it very clear in the docs.
There was a problem hiding this comment.
Oh, I interpreted it as "inserts" meaning only new fields, and the new flag COUNT_INSERTS means "count the inserts in addition to the updates".
So yeah, it seems these docs need to be improved. We could also rename the flag. Any suggestion?
There was a problem hiding this comment.
COUNT_ALL? (meaning inserted, updated/replaced and deleted fields)
There was a problem hiding this comment.
ok.. better than INSERTS. IMHO. or we can wait for other suggestions.
p.s. there are other references to "updates" and "inserts" in the text, we need to make sure they don't get out of context.
|
What are we proposing as a way to write a backwards compatible module? |
|
i don't think we can.. |
... or set errno = 0 before HashSet and check it again afterwards. Then, if errno == 0 after HashSet returned 0, it must be an old Redis. (It's also quite easy to avoid the errors by doing open-for-write and check-key-type before the call to HashSet.) |
|
@zuiderkwast That would work for error conditions, if I'm OK with accepting this limitation but we'll need to document it clearly. |
Good, I agree. I mentioned (added in Redis 6.2) for both the flag and errno. Do you have any more suggestion how to document it clearly? |
|
@zuiderkwast I imagine something in the spirit of "NOTICE: The return value semantics of this function is very different between Redis 6.2 and older versions. Modules that use it should determine the Redis version and handle it accordingly." |
oranagra
changed the title
|
@redis/core-team please approve small module API change to allow modules to make sense of the return value of RM_HashSet |
oranagra
added
approval-needed
The added flag affects the return value of RM_HashSet() to include the number of inserted fields, in addition to updated and deleted fields. Errno is set on errors, tests are added and documentation updated. Fixes redis#6914.
zuiderkwast
force-pushed
the
modules-hashset-flag
branch
from
6796031 to
6a330f5
Compare
|
Possible breakage in ~1971 LoC: https://github.com/search?l=C&q=RedisModule_HashSet&type=Code |
The added flag affects the return value of RM_HashSet() to include the number of inserted fields, in addition to updated and deleted fields. errno is set on errors, tests are added and documentation updated.
5 participants
The added flag affects the return value of RM_HashSet() to include
the number of inserted fields, in addition to updated and deleted
fields.
Errno is set on errors, tests are added and documentation updated.
Fixes #6914.