[
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)
