[Asterisk-code-review] res pjsip session Added rtcp stats result vector into the se... (asterisk[master])

[sungtae kim]

sungtae kim has posted comments on this change. ( https://gerrit.asterisk.org/10899 )

Change subject: res_pjsip_session Added rtcp stats result vector into the session
......................................................................


Patch Set 6:

(1 comment)

> (1 comment)
 > 
 > I don't think in off-nominal error cases the stats should be saved.
 > I think it is only when a media session is terminated after being
 > used that the stats in regards to it should be saved. As I
 > previously said the way it is now can result in stats for RTP
 > sessions that were never used, as well as stats for the same RTP
 > session at different points in time.
 > 
 > As a consumer of the functionality I think trying to use the data
 > would be problematic in its current state. You would need to go
 > through the data, correlate any duplicates, determine if any should
 > be ignored, and then it would be useful. Without doing so it can
 > lead you to wrong interpretations. For example if stats for an RTP
 > instance that was never used were present and I saw the statistics
 > in the vector I might wonder if there was a problem - but in
 > reality there wasn't, because the stream was never actually used.

Hi Joshua,

Thank you for your kind reviewing.

Yes, I've removed some off-nominal error cases. But to be honestly, I haven't look the pjsip code in deep level, so, I doubting about what I've done it. And I'm worry about may I couldn't catch up what you mentioned. So please give me a feedback if I was wrong(I think I made mistake again). I would fix it and learn something from that. :)


Thank you.

Kind regards,
Sungtae

https://gerrit.asterisk.org/#/c/10899/5/include/asterisk/res_pjsip_session.h
File include/asterisk/res_pjsip_session.h:

https://gerrit.asterisk.org/#/c/10899/5/include/asterisk/res_pjsip_session.h@144
PS5, Line 144: };
             : 
> The reason I mentioned passing this in, instead of storing it is because it alters the API and rippl […]
Now I've got it. I was focused on the function more than structure. But this case, yes, you're right. The isolated structure is much more important. I've fixed it. :)



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

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib25c2d3fc4da084aecfde2a82c1b1d733bd64fa5
Gerrit-Change-Number: 10899
Gerrit-PatchSet: 6
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation (1000185)
Gerrit-Reviewer: Joshua C. Colp <jcolp at digium.com>
Gerrit-Reviewer: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Mon, 11 Feb 2019 21:12:35 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20190211/04ab52ec/attachment.html>