jdiameter icon indicating copy to clipboard operation
jdiameter copied to clipboard

Published 20 hours ago • verified publisher icon RestComm

Sy Protocol Implementation

Open aferreiraguido opened this issue 8 years ago • 8 comments

aferreiraguido avatar Sep 08 '16 16:09 aferreiraguido

Related to issue #44

FerUy avatar Sep 08 '16 16:09 FerUy

Thanks @aferreiraguido !

@brainslog @FerUy can you review ?

deruelle avatar Sep 08 '16 16:09 deruelle

Hi @deruelle, work is not finished yet, right @aferreiraguido ?

FerUy avatar Sep 08 '16 16:09 FerUy

actually not. there was added api interfaces and AVP definitions to dictionary.xml.- Yet to be implemented is Common, Client and Server java code.

aferreiraguido avatar Sep 08 '16 21:09 aferreiraguido

Understood. Thanks for the feedback @aferreiraguido

FerUy avatar Sep 08 '16 21:09 FerUy

Please see this is not a request for a merge, the Sy implementation is missing, I thought this pull will allow @brainslog to review the work done until now, as I need some feedback to be sure about the implementation on Server, Client and Common classes as well.

aferreiraguido avatar Sep 08 '16 22:09 aferreiraguido

@aferreiraguido, good job so far! Thanks for the hard work.

I have made some inline comments in the changes, please review them and let me know your thoughts and/or fix where applicable.

brainslog avatar Sep 19 '16 03:09 brainslog

Hi @aferreiraguido!

Sorry for the late review, but here are some of my notes. I thought about doing them inline, but I think it's easier to follow on a single comment, let me know if you have any doubts regarding what I am referring to!

  • @ Session and Listeners, [send/do]FinalSpendingLimit[Request/Answer] should be [send/do]SessionTermination[Request/Answer], to follow the name of the actual Diameter message.

  • Replace getSLRequestTypeAVPValue with getSLRequestType, as this simplifies the API.. we are not strict on this, as we have some get<AVPName>AVPValue, but I prefer if we don't add the suffix.

  • Don't change (un)registerAppFacory to (un)registerAppFactory (we know about this typo, but changing it will break many apps).

  • Also, please don't make unrelated changes in other places (eg, https://github.com/RestComm/jdiameter/pull/57/files#diff-2ea999abd5aad58a6283ef3a04cbe3dc, https://github.com/RestComm/jdiameter/pull/57/files#diff-ea50086ddc217fd882f3bc48efbd9bbe, several dictionary changes.. when you find these unrelated issues, please address them in a separate PR, so it's easier to review and merge)

  • ISyMessageFactory misses the methods for creating answers ?

  • getSLRequestTypeAVPValue always return 0 ?

  • Missing checks for null/invalid sessionData, listener, appIds values at ServerSySessionImpl constructor

  • ServerSySessionImpl is overriding methods from parent implementations with empty bodies (addStateChangeNotification, removeStateChangeNotification, getSessions, getSessionAppId, etc..)

  • ServerSySessionImpl : handleEvent : case SENT_INITIAL_RESPONSE is sending the message again instead of calling dispatchEvent(..) ?

  • ServerSySessionImpl is missing proper AnswerDelivery implementation

  • Is the ClientSySessionImpl missing (among other things?)

Overall it looks like this is still unfinished work, are you still open to finish this work ? I will be more active now on Diameter and able to provide feedback and help you quicker.

brainslog avatar Mar 20 '17 19:03 brainslog