redablooms icon indicating copy to clipboard operation
redablooms copied to clipboard

Dot should be removed in SBF.ADD

Open ramprasadp opened this issue 8 years ago • 11 comments

A command like SBFADD would work with most language drivers without any issue. For eg in perl $redis->sbfadd() works great

Why should we have a DOT .. Else drivers need to be changed

ramprasadp avatar Apr 14 '17 13:04 ramprasadp

The reasoning for this nomenclature is in the Redis modules API docs (https://github.com/antirez/redis/blob/unstable/src/modules/INTRO.md#the-simplest-module-you-can-write):

Note that it is a good idea for modules to call commands with the name of the module followed by a dot, and finally the command name, like in the case of HELLOWORLD.RAND. This way it is less likely to have collisions.

Most if not all existing modules that I'm aware of follow that recommendation.

That said, I am not familiar with Perl or its Redis clients. It is entirely possible that clients would have to be updated to support modules, depending on how the client is implemented. Which specific client are you referring to?

itamarhaber avatar Apr 14 '17 15:04 itamarhaber

I am using perl Redis.pm http://search.cpan.org/~dams/Redis-1.991/lib/Redis.pm. If DOT is recommended then the drivers have to be written to accomodate that way. Actually I recompiled redablooms after removing the "." and now the bloomfilter is working great. But I dont know if that was the right thing to do

ramprasadp avatar Apr 15 '17 06:04 ramprasadp

First of all, feel free to hack on this - it is an early experiment that can benefit from some attention ;) Secondly, the point (dot) you're raising is important in general, so it would be good to have some more discussion on it - perhaps @dvirsky, @melo and/or @antirez can chime in.

itamarhaber avatar Apr 15 '17 13:04 itamarhaber

The dot is to make namespacing easier for module as @itamarhaber already explained. Name spacing is important to make sure we can introduce new core verbs without conflicts with modules in the wild.

I wrote the initial Redis.pm version, and the mechanism that Perl uses to automagically support all commands (AUTOLOAD method) will not work with "." commands as perl names cannot have dots in them. So yes, having a dot as namespace separator for modules is a pain for Perl (not sure about the others).

My first suggestion would be to switch the namespace separator to "" (underscore). This will mean that we still have clear namespaces, but at the same time, "" is usually accepted as a "name" on most languages I know.

A second suggestion, one that I leave more as a food-for-thought, maybe a recommendation to module authors to provide a standard compilation options for all modules that allows users to switch the separator to something else. Either that, or change the API to make the namespace explicit (registering a function will require two strings, namespace and verb) and let the separator be a operator decision. The last option would allow binaries of the modules to be distributed (via yum/apt repositories) while still leaving the option to change the separator as an operator configuration.

@ramprasadp You compiled the module without the ".". I suggest you recompile it with "_" as separator. At least Redis.pm should be fine with that.

melo avatar Apr 18 '17 09:04 melo

Hello, yep in retrospect this is an issue, probably to switch separator is the simplest thing... I guess that - does not work as well since it's an operator, so _ looks like the most sensible thing as @melo suggested. I can update the modules doc about that, but then there is also to ping modules authors in order to change naming schema in the next versions: they can just register both names if they want to stay backward compatible. Looks ok?

antirez avatar Apr 18 '17 14:04 antirez

This is a private Perl case, I would not change anything in the modules, not now that there are modules in actual use. Switching to underscore should just be done as something in that specific client - just parse the string returned from the discovery and translate whatever you want. In fact, the dot notation can be exploited for really cool auto stuff in languages like python and js, and it looks way better than underscores. I would solve the client's problem and get on with it.

dvirsky avatar Apr 25 '17 20:04 dvirsky

As long as Redis provides the necessary discovery, I have no issues with that. As long as I can, as a redis client, discover the commands that exist, and the separator, this is easy peasy to do on Perl side.

But without discovery, this is still a issue. Maybe just for Perl, but an issue, and it needs at least some official guidance.

The COMMAND command should be enough as long as it includes all module commands, I don't know if it does. If it does, I would recommend at least adding a command flag to mention it as an external or module command.

But if they are present, I can send a PR to the Perl client (at least Redis.pm) to allow that a command like MODULE.COMMAND becomes in Perl $redis->module->command... It requires the Redis client to issue a COMMAND command on every connect, but that can be an option on the constructor.

Finally, I don't know if this is a pure-perl issue or not, it manifested itself with the Perl client, but I don't know all the languages that have Redis clients and how they usually do it. It manifested itself with the Perl client because the . is a reserved character that cannot be used in a method name and Perl users are use to calling Redis commands as method names. Today, you can actually use $redis->call('WHATEVER.COMMAND, @args)` if memory serves, so there is even a valid workaround.

melo avatar Apr 26 '17 09:04 melo

@melo you do get them in COMMAND, but besides the dot thing, there is no flag stating that this is a module command (the arity being -1 is a hint since we don't know the command arity in advance in modules). Here's what you get:

169) 1) "ft.search"
     2) (integer) -1
     3) 1) readonly
        2) denyoom
     4) (integer) 1
     5) (integer) 1
     6) (integer) 1

dvirsky avatar Apr 26 '17 10:04 dvirsky

@antirez how about adding a "module" to the flags array?

dvirsky avatar Apr 26 '17 10:04 dvirsky

I'll discuss supporting submodules commands on Redis.pm using COMMAND then. Thanks.

melo avatar Apr 26 '17 11:04 melo

@itamarhaber so we're closing this issue as WONTFIX?

dvirsky avatar Apr 26 '17 12:04 dvirsky