[Asterisk-code-review] res_hiredis: REDIS DeviceState and Dialplan functions (asterisk[master])

[N A]

Attention is currently required from: Sean Bright, Alec Davis.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18829 )

Change subject: res_hiredis: REDIS DeviceState and Dialplan functions
......................................................................


Patch Set 22: Code-Review-1

(7 comments)

Patchset:

PS22: 
> patchset 21/22: […]
Why do we no longer support unload??

This is not part of the core, unless there is a very good reason not to, I think the module should be able to be cleanly unloaded. When I see modules that don't cleanly unload, it usually indicates design issues internally.


File configs/samples/hiredis.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/4feca789_e50ec970 
PS22, Line 2: enabled = yes                   ; When set to yes, hiredis support is enabled, default = no
Since this is uncommented, this would default enable the support for anyone loading the module. Might want to disable it by default? (leave uncommented?)


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/ba07664f_3d67a23b 
PS22, Line 7: prefix = pabx:                  ; Prefix to prepend to all keypaths, default = ""
I'm assuming PABX = Private Automatic Branch Exchange? Just seems like an odd prefix choice in a sample, maybe something more generic?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/cfe55c82_5bdd4542 
PS22, Line 11: background_save_on_unload = yes ; When set to yes, initiate a Background save when unloading module, default=no
So you indicated you remove the unload option but this is still here - would this still be relevant?


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/7dd967d2_0d080420 
PS22, Line 15: ;password = 12345678		; default = "", no authentication
The other lines are indented using spaces but this uses tabs so the alignment is off


File res/res_hiredis.c:

https://gerrit.asterisk.org/c/asterisk/+/18829/comment/805bb0f5_93cc175c 
PS22, Line 54: 			<example>
The examples should have titles for each one that describe what they do, e.g. <example title="Make a direct Redis call">


https://gerrit.asterisk.org/c/asterisk/+/18829/comment/c53fc69c_b11c467e 
PS22, Line 854: 	return -1;
I think the module should be able to be unloaded, this creates a poor user experience



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18829
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: Ifdf4d33d2a2a5fd044fac13b201c7437de34ba6a
Gerrit-Change-Number: 18829
Gerrit-PatchSet: 22
Gerrit-Owner: Alec Davis <alec at bdt.co.nz>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Alec Davis <alec at bdt.co.nz>
Gerrit-Comment-Date: Thu, 04 Aug 2022 13:17:32 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Alec Davis <alec at bdt.co.nz>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220804/d1ef5e0e/attachment-0001.html>