Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add event admin remote provider based on mqtt #773

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

xuzhenbao
Copy link
Contributor

1.To deliver remote events, the celix_event_remote_provider_service_t service has been added , the relationship between event_admin and remote_provider is described in README.md.

2.Add a remote provider bundle based on MQTT, refer to the README.md for the implementation.

xuzhenbao added 23 commits June 18, 2024 20:33
…te_event_admin

# Conflicts:
#	bundles/event_admin/event_admin/gtest/src/CelixEventAdminTestSuite.cc
#	bundles/event_admin/event_admin/gtest/src/CelixEventAdminTestSuiteBaseClass.h
#	libs/utils/error_injector/celix_properties/CMakeLists.txt
#	libs/utils/error_injector/celix_properties/include/celix_properties_ei.h
#	libs/utils/error_injector/celix_properties/src/celix_properties_ei.cc
@pnoltes
Copy link
Contributor

pnoltes commented Nov 10, 2024

@xuzhenbao Thanks for the PR. I am planning to review this, but this will require some time.

@xuzhenbao
Copy link
Contributor Author

@xuzhenbao Thanks for the PR. I am planning to review this, but this will require some time.
Thank you for taking the time to help me review the PR.

Copy link
Contributor

@pnoltes pnoltes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see remote events coming together.

I still need to review the mqtt remote provider and zeroconf implementation, but I already reviewed the libs changes and the changes to the event admin implementation.

bundles/event_admin/event_admin/src/celix_event_admin.c Outdated Show resolved Hide resolved
bundles/event_admin/event_admin/src/celix_event_admin.c Outdated Show resolved Hide resolved
bundles/event_admin/event_admin/src/celix_event_admin.c Outdated Show resolved Hide resolved
bundles/event_admin/event_admin/src/celix_event_admin.c Outdated Show resolved Hide resolved
return ENOMEM;
}
celix_properties_unset(remoteProps, CELIX_EVENT_REMOTE_ENABLE);
celix_status_t status = celix_properties_set(remoteProps, CELIX_EVENT_REMOTE_FRAMEWORK_UUID, ea->fwUUID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I like the design of an opt-in with CELIX_EVENT_REMOTE_ENABLE and the "this is a remote event" indication with CELIX_EVENT_REMOTE_FRAMEWORK_UUID.

bundles/event_admin/event_admin/src/celix_event_admin.c Outdated Show resolved Hide resolved
bundles/event_admin/examples/res/mosquitto.conf Outdated Show resolved Hide resolved
Copy link
Contributor

@pnoltes pnoltes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I review the code. Overall looks good to me, I had some small remarks.

IMO overall the usage of mosquitto is well abstracted.

celix_arrayList_removeAt(deliverer->syncEventQueue, 0);
}
celixThreadMutex_unlock(&deliverer->mutex);
if (entry != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice send outside the lock

@xuzhenbao xuzhenbao requested a review from pnoltes December 6, 2024 03:56
@PengZheng PengZheng removed their request for review December 6, 2024 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants