Add module data-type support for COPY.#8112
Conversation
This adds a copy callback for module data types, in order to make modules compatible with the new COPY command. The callback is optional and COPY will fail for keys with data types that do not implement it.
oranagra
left a comment
oranagra
left a comment
There was a problem hiding this comment.
@MeirShpilraien please take a look.
i suppose a context and auto-memory doesn't apply here.
src/db.c
Outdated
| addReplyError(c, "Copying module type object is not supported"); | ||
| return; | ||
| case OBJ_MODULE: { | ||
| moduleValue *mv = o->ptr; |
There was a problem hiding this comment.
Sort of a nitpick, but we don't encapsulate module logic that well. It seems like this should be closer to:
case OBJ_MODULE:
newobj = moduleDupOrReply(c, o);
if (!newobj) return;
break;
There was a problem hiding this comment.
I'm actually happy with this kind of nitpicking, it's not consistent with how it's done else where but heck, LIBTYFI.
There was a problem hiding this comment.
I agree that's how it's done elsewhere, but I think that is probably an area worth refactoring too :)
| * RedisModuleEvent_FlushDB to hook into that. | ||
| * * **copy**: A callback function pointer that is used to make a copy of the specified key. | ||
| * The module is expected to perform a deep copy of the specified value and return it. | ||
| * In addition, hints about the names of the source and destination keys is provided. |
There was a problem hiding this comment.
Why are hints provided? It seems like it is breaking it's not information the module should need, since keys shouldn't really know where they are. Since you can move them around after they are copied and achieve the same effect.
There was a problem hiding this comment.
we've come to realize that a lot of modules do things they shouldn't!
one case that seems semi-common is storing extra metadata on keys outside of the keyspace.
simple modules will not use that information, but the complex ones may depend on it.
p.s. these modules go through hoops to overcome the absence of the key name in the free callback, and we recently added it for the unlink callback.
There was a problem hiding this comment.
That's also why I refer to this as a hint rather than a hard commitment to provide a key name. We also have the same thing in rdb_save and rdb_load where the key name can be queried from RedisModuleIO but it may or may not be there depending on the context.
There was a problem hiding this comment.
Yeah, the whole notion that we added a "hint" is sort of weird. I don't know why anyone would actually want to rely on it. If they want to store additional metadata outside of the keyspace, it seems like the notification system is a better approach?
|
@redis/core-team please approve |
oranagra
added
the
release-notes
This adds a copy callback for module data types, in order to make modules compatible with the new COPY command. The callback is optional and COPY will fail for keys with data types that do not implement it.
5 participants
This adds a copy callback for module data types, in order to make
modules compatible with the new COPY command.
The callback is optional and COPY will fail for keys with data types
that do not implement it.