Event admin #738
Event admin #738
Conversation
…t_admin # Conflicts: # libs/framework/error_injector/celix_bundle_ctx/CMakeLists.txt # libs/framework/error_injector/celix_bundle_ctx/include/celix_bundle_context_ei.h # libs/framework/error_injector/celix_bundle_ctx/src/celix_bundle_context_ei.cc # libs/framework/include/celix_dm_component.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 # misc/experimental/bundles/CMakeLists.txt
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #738 +/- ##
==========================================
+ Coverage 89.58% 89.88% +0.30%
==========================================
Files 216 222 +6
Lines 25413 26092 +679
==========================================
+ Hits 22765 23454 +689
+ Misses 2648 2638 -10 ☔ View full report in Codecov by Sentry. |
|
Very nice addition for Apache Celix :). I had a short look into the event_admin_api and this looks good; Clean api and well documented. I will try to find some time this week and next week to more thoroughly review this. |
bundles/event_admin/event_admin_api/include/celix_event_admin_service.h
Outdated
Show resolved
Hide resolved
bundles/event_admin/event_admin_api/include/celix_event_admin_service.h
Outdated
Show resolved
Hide resolved
bundles/event_admin/event_admin_api/include/celix_event_constants.h
Outdated
Show resolved
Hide resolved
bundles/event_admin/event_admin_api/include/celix_event_constants.h
Outdated
Show resolved
Hide resolved
…service.h Co-authored-by: PengZheng <[email protected]>
|
|
||
| | Celix Framework Condition ID | Event Admin Event topic | | ||
| |------------------------------------|---------------------------------------------| | ||
| | CELIX_CONDITION_ID_FRAMEWORK_READY | "org/osgi/framework/FrameworkEvent/STARTED" | |
There was a problem hiding this comment.
Good choice and prevents additional needs to exposing a additional framework header.
| int celix_eventAdmin_start(celix_event_admin_t* ea) { | ||
| celix_status_t status = CELIX_SUCCESS; | ||
| ea->threadsRunning = true; | ||
| for (int thCnt = 0; thCnt < CELIX_EVENT_ADMIN_MAX_HANDLER_THREADS; ++thCnt) { |
There was a problem hiding this comment.
nitpick:
The name of CELIX_EVENT_ADMIN_MAX_HANDLER_THREADS implies that the event admin spins up a maximum of CELIX_EVENT_ADMIN_MAX_HANDLER_THREADS threads, but these threads are always created (in the happy flow).
I think the name should imply this, something like: CELIX_EVENT_ADMIN_HANDLER_THREADS.
I also think this is a good candidate for a configuration parameter, so that users can lower or increase the event handlers threads based on their usage.
| } | ||
| return status; | ||
| } | ||
| celixThread_setName(&ea->eventHandlerThreads[thCnt], "EventHandler"); |
There was a problem hiding this comment.
Good that the threads have a name, The Celix Event is named "CelixEvent".
Maybe this could be named "EventAdmin-1", "EventAdmin-2", etc. Still shorts (< 16 chars) but more specific for the event admin.
| /** | ||
| * @brief The Event Admin service name | ||
| */ | ||
| #define CELIX_EVENT_ADMIN_SERVICE_NAME "org.osgi.service.event.EventAdmin" |
There was a problem hiding this comment.
Although Apache Celix is inspired by OSGi and we did use some OSGi named constants, enums etc.
I currently think it is better not to use the "org.osgi" or "osgi" part and replace that with "celix".
So in this case "celix.service.event.EventAdmin"
There was a problem hiding this comment.
nice examples. Shows the simplicity (in usage) of the event admin.
|
|
||
| | Celix Bundle Event Type | Event Admin Event topic | | ||
| |--------------------------------|----------------------------------------------| | ||
| | CELIX_BUNDLE_EVENT_INSTALLED | "org/osgi/framework/BundleEvent/INSTALLED" | |
There was a problem hiding this comment.
Also mentioned for the service names, but I prefer that "org.osgi" or "osgi" is not used (inspired by, but not a implementation of).
So I would prefer topics like "celix/framework/BundleEvent/INSTALLED
…eventAdmin_create
This PR implements a local event admin and maps celix-framework-event to event-admin-event. See the PR's README.md for details.