6 Dec
2019
6 Dec
'19
5:02 p.m.
#### Pre-Submission Checklist
<!-- Go over all points below, and after creating the PR, tick all the checkboxes
that apply -->
<!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines
from above-->
<!-- If you're unsure about any of these, don't hesitate to ask on
sr-dev mailing list -->
- [x] Commit message has the format required by CONTRIBUTING guide
- [x] Commits are split per component (core, individual modules, libs, utils, ...)
- [x] Each component has a single commit (if not, squash them into one commit)
- [x] No commits to README files for modules (changes must be done to docbook files
in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change
- [ ] Small bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds new functionality)
- [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist:
<!-- Go over all points below, and after creating the PR, tick the checkboxes that
apply -->
- [ ] PR should be backported to stable branches
- [x] Tested changes locally
- [x] Related to PR #2162
#### Description
As discussed on #2162 with this change it's possible to decide witch xavp_rcd
values will be stored using the new module parameter xavp_rcd_mask
Using the same helper function on build_contact() so the behavior is consistant. Before
only ruid and expires where stored when build_contact() was called.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/2168
-- Commit Summary --
* registrar: control what values to add to xavp_rcd via xavp_rcd_mask
* registrar: add documentation for xavp_rcd_mask parameter
-- File Changes --
M src/modules/registrar/doc/registrar_admin.xml (31)
M src/modules/registrar/lookup.c (35)
M src/modules/registrar/lookup.h (4)
M src/modules/registrar/registrar.c (3)
M src/modules/registrar/registrar.h (8)
M src/modules/registrar/reply.c (47)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/2168.patch
https://github.com/kamailio/kamailio/pull/2168.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168
9 Dec
9 Dec
2:57 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
Overall I am fine with the PR, but because of limited time to test, I am not sure if there
is same result now that the lookup of `list` and `xavp` is no longer done only once in
`build_contact()`. In previous version the attributes for all contacts were done inside
`build_contact()` using this single lookup. Now the lookup is done for each contact inside
the helper function.
I expect is the same, based on quick thinking. If you did the tests and the result is the
same in terms of what is stored in xavp and the order of values, then I am fine to merge.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563204545
3:37 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@linuxmaniac i think the default should be set to the value that makes it work like in
5.3
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563218964
3:41 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@lazdo - I think it would be more user friendly this way. If one is concerned about the
performance implication it is easy to deactivate it. Or do you have other concerns here?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563220317
5:12 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@henningw just saw `Defines what values to skip when xavp_rcd is stored.` which i think
its the wrong approach. imo, it should define what values to retrieve and default that to
the existing ones in 5.3 branch. with this implementation, if new values are added they
are retrieved by default which may increase the memory footprint and take more time to
process it (shouldn't be relevant).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563255674
6:37 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@lazedo We don't have a policy to keep the memory requirements completely equal during
major releases. Otherwise it would be not possible to do non-trivial extensions to our
code-base. All kind of extensions done in the core or modules might slighly increase
memory and/or run-time for this particular operation. (That said - I don't think that
is most of them are noticable or even significanly measurable.) You can easily disable
this feature it in your installation if you are concerned about it.
Daniel already agreed to merge it, I also think we should merge it this way.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563295736
8:01 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@henningw `You can easily disable this feature it in your installation if you are
concerned about it` this is exactly what you should not ask the user to do, no ?
i'm not blocking this in any way, just concerned.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563333771
8:55 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@lazedo - actually the PR saving new field in xavp is merged. I suggested at that time to
add an option to control what is added, because more and more fields were wanted in the
xavp and they were stored by implicitly -- that was the what every developer did before.
So all these were following the previous pattern - save implicitly. With this new PR there
is an option for fine control. I think it is better to have a straight rule -- with
default option: save all fields or don't save any, then this parameter comes to do
selective decision on saving specific fields. The preference was to store all and have
option to skip -- from my point of view could have been also the opposite: don't store
any by default and specify what to store.
I think it would be a bit more confusing/unnatural to have a "default random list of
stored attributes".
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563355457
9:03 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@miconda i'm not proposing `"default random list of stored attributes"`, i
`was` proposing a known value that is compatible with 5.3 `if` it was implemented by
`adding`instead of `skipping`.
but as i said before, i'm not blocking this in any way, just concerned about memory
footprint from one version to another as a principle, tho in this is case, it adds almost
none
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563358740
9:47 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
I am not sure if there is same result now that the
lookup of `list` and `xavp` is no longer done only once in `build_contact()`. In previous
version the attributes for all contacts were done inside `build_contact()` using this
single lookup. Now the lookup is done for each contact inside the helper function.
I expect is the same, based on quick thinking. If you did the tests and the result is the
same in terms of what is stored in xavp and the order of values, then I am fine to merge.
I'm going to do a unit for that in http://www.github.com/kamailio/kamailio-tests so We
can be sure. Holding this until then.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563376307
10:58 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
@lazedo - I used "random" for the personal opinions/needs of the developers
which added storage of the fields in the past, not being a decision done based on a
discussion devs and evaluation of the impact. If resource usage was a concern, that should
have been with the first attribute added, then it was also an impact on memory usage
between kamailio major versions.
The approach was to add new attributes as some dev had new needs. Because the list seemed
to grow over the time, it made sense to add an option to control in long term. But from
consistency point of view it should be one way or the other: store all by default and
allow option to skip some; or just don't add any and allow option to specify which to
store.
This new parameter it doesn't change the existing behaviour of the master branch
before merging the PR -- again, at this moment in core all the attributes wanted are
stored implicitly. Once merging this PR, all the attributes are still stored, so it
doesn't change the default behavior. Just add option to be able to skip some via
modparam.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-563409103
11 Dec
11 Dec
11:51 a.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
Tested that xavp_rcd has the same order with this change, for sure now it will have more
values depending of the value of xavp_rcd_mask
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-564441166
1:34 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
OK, thanks, it can be merged.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#issuecomment-564480661
1:40 p.m.
New subject: [kamailio/kamailio] registrar: add new xavp_rcd_mask parameter (#2168)
Merged #2168 into master.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/2168#event-2873785447
1812
days inactive
1817
days old
13 comments
4 participants
participants (4)
-
Daniel-Constantin Mierla -
Henning Westerholt
-
lazedo
-
Victor Seva