vetss
commented
7 years ago Hello @vmykhailiuk
thanks for your work for SCCP connected oriented messages implementeation. I have added you PR https://github.com/RestComm/jss7/pull/240 to a master branch and hope for finishing of the implementation.
See my comments below:
1) please pay attention on code style checking (you can just run "mvn clean install" command to check it). See my extra update https://github.com/RestComm/jss7/commit/37443e4ec69516257fb7e5b6756a5db80280c4a5 to understand a reason. Also we have code style demands for contributors, you can check the following link: https://github.com/RestComm/parent/wiki/ContributeToMobicents
2) which parts of SCCP connected-oriented staff are you going to implement (from Q.711): a) permanent connecions b) temporary connections b1) X.213 style b2) SUP-embedded c) implementation of N-RESET support
3) You added a SccpConnMessage interface that looks like a representation of all connected-oriented messages. Better to implement a separate interface per a message. We can extend SccpMessage as a base interface.
4) We have MessageFactoryImpl class that should be used for message creating. It needs to be updated for it.
5)
- SegmentingReassembling - better to have a parameter like boolean isModeData() instead of just "getValue()" to be more clear
- there are several parameters that may have reserved values (not fully specified by a spec) like ReleaseCause (not only this one). We need to have in interfaces both:
ReleaseCauseValue getValue();
int getIntValue();(and two types of constructors / setters). getValue() will return a value only for values that are present in spec, getIntValue(); will return value in all cases
6) We use now a file header with "TeleStax" company name (instead of "jboss") for added files, example is : https://github.com/RestComm/jss7/blob/master/map/map-api/src/main/java/org/mobicents/protocols/ss7/map/api/primitives/TMSI.java Please use this style for your further implementation
7) Just for info - we reuse now for connection-less messages a special parameter networkId for identifying of remote network to which we have a connection (it is wide used for message routing). Please add support for this into connected-oriented messages. You can check info in: http://documentation.telestax.com/core/ss7/SS7_Stack_User_Guide.html#_design_multitenancy
8) We need to implement unit tests for checking of (de)serializing of messages / parameters and for further messageflow like we have now for connectionless operations. The first part you are adding is - messages / parameters so we will need to add unit tests firtsly for them. It will be the best if you have message examples form live traces and will be able to implent such tests based on this.
vmykhailiuk
Currently only connectionless protocol classes are supported. It's needed to add message class for connection-oriented protocol class messages and add classes for parameters used in them.
Going to create pull request (major patch) with corresponding changes.