[jira] [Comment Edited] (CELIX-407) Extract pubsub_serializer from pubsub_admin

["Pepijn Noltes (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CELIX-407?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15957469#comment-15957469 ] 

Pepijn Noltes edited comment on CELIX-407 at 4/5/17 7:12 PM:
-------------------------------------------------------------

Roy,

First of all nice work! i really like that we now have a separate service for serialisation. A few comments / questions based on scanning/reviewing a bit of the work:

- How does serialisers work? 

Is it  possible to have multiple serialisers. If so, how does the selection algorithm works. Also if i am correct at the moment the used serialisation is not part of the topic_publication (endpoint data) discovery data? I think this is needed to ensure that publishers and subscribers can communicate.

My proposal would be to allow multiple serializers, but to use service ranking to select to serializer used (publication) . For subscription the serializer can be selected based on the endpoint properties of the publishers. This also means that the serialiser should have some meta properties to discriminate the different serializers. e.g. “serializer.config=json/dfi/v1” 


- I think it would be more correct if the psa->setSerializer is called psa->addSerializer, because you can have multiple serialisers.

- I prefer the handle of the service to be called handle and be a void* type. I known this is very type unsafe, but IMO it does clearly communicate that a) it is a opaque handle and b) the user should not worry about it actual type. Also because most services used this, this should be a clear and recognisable pattern .

- For the pubsub_message_type, i do think it should be an opaque struct. But would prefer a bit a documentation stating this. Also document the key and value of the hashmap

- The serialiser uses a fillMsgTypeMap function (and some other util functions) from dyn_msg_utils. It could use a prefix to make it’s origin more clear and prevent name clashes. e.g. dynMsgUtils_fillMsgTypeMap.

Again nice works, just some feedback. Let me known what you think.



was (Author: pnoltes):
Roy,

First of all nice work! i really like that we now have a separate service for serialisation. A few comments / questions based on scanning/reviewing a bit of the work:

- How does serialisers work? 

Is it  possible to have multiple serialisers. If how does the selection algorithm works. Also if i am correct at the moment the used serialisation is not part of the topic_publication (endpoint data) discovery data? I think this is needed to ensure they can communicate.

My proposal would be to allow multiple serializers, but to use service ranking to select to serializer used (publication) . For subscription the serializer can be selected based on the one used by the publishers. This also means that the serialiser should have some meta properties to discriminate the different serializers. e.g. “serializer.config=json/dfi/v1” 


- I think it would be more correct if the psa->setSerializer is called psa->addSerializer, because you can have multiple serialisers.

- I prefer the handle of the service to be called handle and be a void* type. I known this is very type unsafe, but IMO it does clearly communicate that a) it is a opaque handle and b) the user should not worry about it actual type. 

- For the pubsub_message_type, i do think it should be an opaque struct. But would prefer a bit a documentation stating this. Also document the key and value of the hashmap

- The serialiser uses a fillMsgTypeMap function (and some other util functions) from dyn_msg_utils. It could use a prefix to make it’s origin more clear and prevent name clashes. e.g. dynMsgUtils_fillMsgTypeMap.

Again nice works, just some feedback. Let me known what you think.


> Extract pubsub_serializer from pubsub_admin
> -------------------------------------------
>
>                 Key: CELIX-407
>                 URL: https://issues.apache.org/jira/browse/CELIX-407
>             Project: Celix
>          Issue Type: Improvement
>            Reporter: Roy Lenferink
>             Fix For: next
>
>
> At the moment, the pubsub_admin is responsible for both communication and serialization. It would be better to extract the serializer from the pubsub_admin.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)