You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@celix.apache.org by GitBox <gi...@apache.org> on 2020/06/17 18:40:29 UTC
[GitHub] [celix] pnoltes opened a new pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
pnoltes opened a new pull request #257:
URL: https://github.com/apache/celix/pull/257
Refactors all pubsub admins to support multiple subscriber services with the same topic/scope combination in a single bundle.
@Oipo Could you also have a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
Posted by GitBox <gi...@apache.org>.
pnoltes commented on pull request #257:
URL: https://github.com/apache/celix/pull/257#issuecomment-645901364
> The changes made so far look good to me, but I think all the senders have the same issue and would need a similar refactoring.
no. a publisher service is registered as service factory. That means every bundle get its own publisher. Multiple components in a single bundle can all use the same service instance.
The issue with subscribers is that all registered subscribers need to be picked up the the topic receivers.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a change in pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #257:
URL: https://github.com/apache/celix/pull/257#discussion_r442463106
##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c
##########
@@ -550,14 +554,19 @@ processMsgForSubscriberEntry(pubsub_tcp_topic_receiver_t *receiver, psa_tcp_subs
}
if (status == CELIX_SUCCESS) {
- bool release = true;
- svc->receive(svc->handle, msgSer->msgName, msgSer->msgId, deSerializedMsg, message->metadata.metadata,
- &release);
- if (release) {
- msgSer->freeDeserializeMsg(msgSer->handle, deSerializedMsg);
+ hash_map_iterator_t iter = hashMapIterator_construct(entry->subscriberServices);
+ while (hashMapIterator_hasNext(&iter)) {
+ bool release = true;
+ pubsub_subscriber_t *svc = hashMapIterator_nextValue(&iter);
+ svc->receive(svc->handle, msgSer->msgName, msgSer->msgId, deSerializedMsg, message->metadata.metadata,
Review comment:
Fixes the release handling. Now if ownership is taken (release == false) a new message will be deserialized.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Oipo commented on pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
Posted by GitBox <gi...@apache.org>.
Oipo commented on pull request #257:
URL: https://github.com/apache/celix/pull/257#issuecomment-646287483
LGTM.
What'd be really desirable is a shared_ptr, I think. But it's C :sweat_smile:.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes commented on a change in pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
Posted by GitBox <gi...@apache.org>.
pnoltes commented on a change in pull request #257:
URL: https://github.com/apache/celix/pull/257#discussion_r442095368
##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c
##########
@@ -550,14 +554,19 @@ processMsgForSubscriberEntry(pubsub_tcp_topic_receiver_t *receiver, psa_tcp_subs
}
if (status == CELIX_SUCCESS) {
- bool release = true;
- svc->receive(svc->handle, msgSer->msgName, msgSer->msgId, deSerializedMsg, message->metadata.metadata,
- &release);
- if (release) {
- msgSer->freeDeserializeMsg(msgSer->handle, deSerializedMsg);
+ hash_map_iterator_t iter = hashMapIterator_construct(entry->subscriberServices);
+ while (hashMapIterator_hasNext(&iter)) {
+ bool release = true;
+ pubsub_subscriber_t *svc = hashMapIterator_nextValue(&iter);
+ svc->receive(svc->handle, msgSer->msgName, msgSer->msgId, deSerializedMsg, message->metadata.metadata,
Review comment:
ouch. Yes this is an issue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] pnoltes merged pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
Posted by GitBox <gi...@apache.org>.
pnoltes merged pull request #257:
URL: https://github.com/apache/celix/pull/257
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [celix] Oipo commented on a change in pull request #257: Refactors all pubsub admins to handle multiple subscribers per bundle
Posted by GitBox <gi...@apache.org>.
Oipo commented on a change in pull request #257:
URL: https://github.com/apache/celix/pull/257#discussion_r442072432
##########
File path: bundles/pubsub/pubsub_admin_tcp/src/pubsub_tcp_topic_receiver.c
##########
@@ -550,14 +554,19 @@ processMsgForSubscriberEntry(pubsub_tcp_topic_receiver_t *receiver, psa_tcp_subs
}
if (status == CELIX_SUCCESS) {
- bool release = true;
- svc->receive(svc->handle, msgSer->msgName, msgSer->msgId, deSerializedMsg, message->metadata.metadata,
- &release);
- if (release) {
- msgSer->freeDeserializeMsg(msgSer->handle, deSerializedMsg);
+ hash_map_iterator_t iter = hashMapIterator_construct(entry->subscriberServices);
+ while (hashMapIterator_hasNext(&iter)) {
+ bool release = true;
+ pubsub_subscriber_t *svc = hashMapIterator_nextValue(&iter);
+ svc->receive(svc->handle, msgSer->msgName, msgSer->msgId, deSerializedMsg, message->metadata.metadata,
Review comment:
What if two subscribers want to take ownership of a message? Of course they have to check the release boolean to see if they're allowed to do that, but hmm...feels icky.
This also goes for releaseMsg in this function.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org