[Asterisk-code-review] res pjsip pubsub: Prune subs with reliable transports at sta... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/8082 )

Change subject: res_pjsip_pubsub: Prune subs with reliable transports at startup
......................................................................


Patch Set 1: Code-Review-1

(14 comments)

https://gerrit.asterisk.org/#/c/8082/1//COMMIT_MSG
Commit Message:

https://gerrit.asterisk.org/#/c/8082/1//COMMIT_MSG@10
PS1, Line 10: were pruned on Asterisk restart since the TCP connection would have
The earlier option is called prune_on_boot not prune_on_restart.  Since we are doing the same thing to subscription contacts as we had to do to registration contacts don't you think we should use the same name?


https://gerrit.asterisk.org/#/c/8082/1/CHANGES
File CHANGES:

https://gerrit.asterisk.org/#/c/8082/1/CHANGES@54
PS1, Line 54:    were pruned on Asterisk restart since the TCP connection would have
The earlier option is called prune_on_boot not prune_on_restart.


https://gerrit.asterisk.org/#/c/8082/1/CHANGES@56
PS1, Line 56:    process is now also applied to inbound subscriptions.  Sicne this
s/Sicne/Since/


https://gerrit.asterisk.org/#/c/8082/1/CHANGES@59
PS1, Line 59:    need to run the "alembic upgrade head" process to add teh column to
s/teh/the/


https://gerrit.asterisk.org/#/c/8082/1/CHANGES@60
PS1, Line 60:    the schema. 
red blob


https://gerrit.asterisk.org/#/c/8082/1/UPGRADE.txt
File UPGRADE.txt:

https://gerrit.asterisk.org/#/c/8082/1/UPGRADE.txt@51
PS1, Line 51: 
stray blank line


https://gerrit.asterisk.org/#/c/8082/1/contrib/ast-db-manage/config/versions/d3e4284f8707_add_prune_on_restart_to_ps_subscription_.py
File contrib/ast-db-manage/config/versions/d3e4284f8707_add_prune_on_restart_to_ps_subscription_.py:

https://gerrit.asterisk.org/#/c/8082/1/contrib/ast-db-manage/config/versions/d3e4284f8707_add_prune_on_restart_to_ps_subscription_.py@28
PS1, Line 28:     op.add_column('ps_subscription_persistence', sa.Column('prune_on_restart', yesno_values))
The earlier option is called prune_on_boot not prune_on_restart.


https://gerrit.asterisk.org/#/c/8082/1/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

https://gerrit.asterisk.org/#/c/8082/1/include/asterisk/res_pjsip.h@3101
PS1, Line 3101: void ast_sip_transport_monitor_unregister_all(	enum ast_transport_monitor_id id,
A stray tab somehow got into this line.
s/ enum/enum/


https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip/pjsip_transport_events.c
File res/res_pjsip/pjsip_transport_events.c:

https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip/pjsip_transport_events.c@44
PS1, Line 44: 	/*! An opaque ID for matching unregisters. */
            : 	enum ast_transport_monitor_id id;
You don't need to add id when you can use the callback address to do the job of the id.

Adding the id needlessly increases module coupling.  You now have to change res_pjsip code simply to add a new module that needs to monitor a transport.  Eventually we'll have to add outbound subscriptions.

Another reason to use the callback address instead of an arbitrary enum is that a matching monitor callback is likely tightly bound to the monitor callback's data.  The monitor callback has to know what the arbitrary data passed to it means just like a custom matching function has to know.


https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_pubsub.c
File res/res_pjsip_pubsub.c:

https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_pubsub.c@654
PS1, Line 654: 						// N.B.  ast_sip_transport_monitor_register holds a reference to the sub_tree
C++ comment
What is N.B.?


https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_pubsub.c@5546
PS1, Line 5546: 	ast_sorcery_object_field_register(sorcery, "subscription_persistence", "prune_on_restart", "0", OPT_UINT_T, 0,
              : 		FLDSET(struct subscription_persistence, prune_on_restart));
The earlier option is called prune_on_boot not prune_on_restart.


https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_registrar.c
File res/res_pjsip_registrar.c:

https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_registrar.c@590
PS1, Line 590: 			int prune_on_boot = 0;
Initialization is no longer needed as we now always set the value below.


https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_registrar_expire.c
File res/res_pjsip_registrar_expire.c:

https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_registrar_expire.c@35
PS1, Line 35: //  These are common with res_pjsip_registrar!
C++ comment


https://gerrit.asterisk.org/#/c/8082/1/res/res_pjsip_registrar_expire.c@90
PS1, Line 90: 			// We don't have a pointer to transport here so we remove from all transports.
C++ comment



-- 
To view, visit https://gerrit.asterisk.org/8082
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee87cf4eb9b7b2b93d5739a72af52d6ca8fbbe36
Gerrit-Change-Number: 8082
Gerrit-PatchSet: 1
Gerrit-Owner: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Mon, 29 Jan 2018 18:39:42 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180129/2d84ce30/attachment.html>