pnoltes commented on code in PR #710:
URL: https://github.com/apache/celix/pull/710#discussion_r1445137264
##########
bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c:
##########
@@ -115,38 +114,23 @@ celix_status_t
discoveryZeroconfAnnouncer_create(celix_bundle_context_t *ctx, ce
celix_autoptr(celix_thread_mutex_t) mutex = &announcer->mutex;
celix_autoptr(celix_string_hash_map_t) endpoints = announcer->endpoints =
celix_stringHashMap_create();
- assert(announcer->endpoints != NULL);
+ if (endpoints == NULL) {
+ celix_logHelper_logTssErrors(logHelper, CELIX_LOG_LEVEL_ERROR);
+ celix_logHelper_fatal(logHelper, "Announcer: Failed to create
endpoints map.");
Review Comment:
I think the log level should be error. This is what we currently describe in
the development guide:
https://github.com/apache/celix/tree/dc0e0aebb282ce05b51631dd5c53b14783b856ba/documents/development#error-handling-and-logging
> error: This level should be used to report issues that need immediate
attention and might prevent the system from functioning correctly. These are
problems that are unexpected and affect functionality, but not so severe that
the process needs to stop. Examples include runtime errors, inability to
connect to a service, etc.
> fatal: Use this level to report severe errors that prevent the program
from continuing to run. After logging a fatal error, the program will typically
terminate.
Optionally we can also update the development guideline if we think
situations like ENOMEM should be reported as fatal.
##########
bundles/remote_services/rsa_spi/include/remote_constants.h:
##########
@@ -37,13 +37,9 @@ static const char * const OSGI_RSA_SERVICE_EXPORTED_CONFIGS
= "service.exported.
static const char * const OSGI_RSA_SERVICE_LOCATION = "service.location";
/**
- * Remote service admin property identifying the network interfaces that
announce service.
- * The property value is network interfaces name, it Can be a comma separated
list of CIDR notation,eg:"eth0,en0".
- * Default value is "", and the loopback interface will be used. Its exported
service will be discovered
- * only by other local clients on the same machine.
- * If want to bound service to all network interfaces, we can set the property
value to "all".
+ * It identify which types of remote service configurations are supported by a
distribution provider.
+ * @ref
https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.remoteservices.html#i1708968
*/
-static const char * const CELIX_RSA_NETWORK_INTERFACES =
"org.apache.celix.rsa.network.interfaces";
-
+#define OSGI_RSA_REMOTE_CONFIGS_SUPPORTED "remote.configs.supported"
Review Comment:
i.e. global replace the OSGI_RSA with CELIX_RSA
##########
bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_activator.c:
##########
@@ -18,43 +18,116 @@
*/
#include "discovery_zeroconf_announcer.h"
#include "discovery_zeroconf_watcher.h"
+#include "endpoint_listener.h"
+#include "remote_constants.h"
+#include "remote_service_admin.h"
#include "celix_log_helper.h"
#include "celix_bundle_activator.h"
-#include "celix_types.h"
+#include "celix_dm_component.h"
+#include "celix_dm_service_dependency.h"
+#include "celix_constants.h"
#include "celix_errno.h"
+#include <stdio.h>
+#include <assert.h>
typedef struct discovery_zeroconf_activator {
celix_log_helper_t *logHelper;
discovery_zeroconf_announcer_t *announcer;
discovery_zeroconf_watcher_t *watcher;
+ endpoint_listener_t endpointListener;
}discovery_zeroconf_activator_t;
celix_status_t discoveryZeroconfActivator_start(discovery_zeroconf_activator_t
*act, celix_bundle_context_t *ctx) {
celix_status_t status = CELIX_SUCCESS;
- celix_autoptr(celix_log_helper_t) logger =
celix_logHelper_create(ctx,"celix_rsa_zeroconf_discovery");
+ const char *fwUuid = celix_bundleContext_getProperty(ctx,
CELIX_FRAMEWORK_UUID, NULL);
+ if (fwUuid == NULL) {
+ return CELIX_BUNDLE_EXCEPTION;
+ }
+ celix_autoptr(celix_log_helper_t) logger = celix_logHelper_create(ctx,
"celix_rsa_zeroconf_discovery");
if (logger == NULL) {
return CELIX_ENOMEM;
}
- celix_autoptr(discovery_zeroconf_announcer_t) announcer = NULL;
- status = discoveryZeroconfAnnouncer_create(ctx, logger, &announcer);
+ //init announcer
+ celix_autoptr(celix_dm_component_t) announcerCmp =
celix_dmComponent_create(ctx, "DZC_ANNOUNCER_CMP");
+ if (announcerCmp == NULL) {
+ return CELIX_ENOMEM;
+ }
+ status = discoveryZeroconfAnnouncer_create(ctx, logger, &act->announcer);
if (status != CELIX_SUCCESS) {
return status;
}
- celix_autoptr(discovery_zeroconf_watcher_t) watcher = NULL;
- status = discoveryZeroconfWatcher_create(ctx, logger, &watcher);
+ celix_dmComponent_setImplementation(announcerCmp, act->announcer);
+ CELIX_DM_COMPONENT_SET_IMPLEMENTATION_DESTROY_FUNCTION(announcerCmp,
discovery_zeroconf_announcer_t, discoveryZeroconfAnnouncer_destroy);
+ celix_properties_t *props = celix_properties_create();
+ if (props == NULL) {
+ return CELIX_ENOMEM;
+ }
+ char scope[256] = {0};
+ (void)snprintf(scope, sizeof(scope), "(&(%s=*)(%s=%s))",
CELIX_FRAMEWORK_SERVICE_NAME,
+ OSGI_RSA_ENDPOINT_FRAMEWORK_UUID, fwUuid);
+ celix_properties_set(props, OSGI_ENDPOINT_LISTENER_SCOPE, scope);
+ celix_properties_set(props, "DISCOVERY", "true");//Only use to avoid the
discovery calls to unnecessary endpoint listener service.Endpoint should be
filtered by the scope.
Review Comment:
Ok. I see this is used in discovery_common /
discovery_endpointListenerAdded. I am not a fan of this approach, but IMO we
can keep this for now.
##########
bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c:
##########
@@ -532,7 +644,8 @@ static void
*discoveryZeroconfAnnouncer_refreshEndpointThread(void *data) {
discoveryZeroconfAnnouncer_handleMDNSEvent(announcer);
}
} else if (result == -1 && errno != EINTR) {
- celix_logHelper_error(announcer->logHelper, "Announcer: Error
Selecting event, %d.", errno);
+ celix_logHelper_error(announcer->logHelper, "Announcer: Error
Selecting event, %d.", errno);
+ sleep(1);//avoid busy loop
Review Comment:
> We can really do nothing locally to recover from this kind of error. This
represents some long lasting exceptional system condition, which can luckily be
modeled using Celix condition. Once some outside watchers get notified of this
system exception, he/she can try to restart the bundle, which could fix the
accidentally closed fd in this case.
>
> I think generic system exception is a generally useful concept, which
further extends error code and C++ exception. Combined with RSA, this means
these exceptions can be watched remotely (cool). Event better, we don't need
descriptors for Remote Celix conditions because they have no methods.
>
> WDYT? @pnoltes @xuzhenbao
I think remote conditions are very useful and indeed this does not need a
descriptor, because conditions are marker interfaces. remote conditions
combined with a dependency management component means that components can react
(start/stop) based on the presence of a condition in a different framework.
Currently I think the RSA expect to find a dfi descriptor, so maybe an
extensions is needed to service exported with descriptors are handled as marker
interface services.
btw of course out of scope for this PR.
##########
bundles/remote_services/remote_service_admin_dfi/src/remote_service_admin_dfi_constants.h:
##########
@@ -36,6 +36,15 @@
#define RSA_DFI_CONFIGURATION_TYPE "org.amdatu.remote.admin.http"
#define RSA_DFI_ENDPOINT_URL "org.amdatu.remote.admin.http.url"
+/**
+ * @brief RSA Configuration type for zeroconf http, it is
synonymous(https://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.remoteservices.html#i1698916)
with RSA_DFI_CONFIGURATION_TYPE, they refer to the same endpoint.
Review Comment:
I understand this different.
As I understand it, the service.exported.configs and
service.imported.configs refer to remote service admin configurations and not
discovery.
Different discovery mechanism should work without knowledge of the remote
service admins used and vica versa.
For example
- RSA DFI adds a "org.amdatu.remote.admin.http.url" property when creating
a endpoint for an export service.
- Discovery (Zeroconf, configured or etcd) "just" announces and discovers
the endpoints.
- RSA DFI checks the config when importing a service through a endpoint and
uses the "org.amdatu.remote.admin.http.url".
It is possible to support configuration and endpoint with a "ifname",
"port", "ipdresses" and "path" next to a "url" property, but IMO this is then
an extension to RSA DFI and not a ZEROCONF concern.
For example (snippet):
```C
if (strncmp(configType, RSA_DFI_CONFIGURATION_TYPE, 1024) == 0) {
const char *url = celix_properties_get(endpoint->properties,
RSA_DFI_ENDPOINT_URL, NULL);
if (url) {
return true; //RSA_DFI config with endpoint url
}
const char *ip = celix_properties_get(endpoint->properties,
CELIX_RSA_DFI_ZEROCONF_ENDPOINT_IPADDRESSES, NULL);//the property is set by the
discovery_zeroconf
const char *port = celix_properties_get(endpoint->properties,
CELIX_RSA_DFI_ZEROCONF_ENDPOINT_PORT, NULL);
const char *path = celix_properties_get(endpoint->properties,
CELIX_RSA_DFI_ZEROCONF_ENDPOINT_PATH, NULL);
if (ip != NULL || port != NULL || path != NULL) {
return true; //RSA_DFI config with a endpoint ip, port and path/
}
}
return false;
```
##########
bundles/remote_services/discovery_zeroconf/src/discovery_zeroconf_announcer.c:
##########
@@ -532,7 +644,8 @@ static void
*discoveryZeroconfAnnouncer_refreshEndpointThread(void *data) {
discoveryZeroconfAnnouncer_handleMDNSEvent(announcer);
}
} else if (result == -1 && errno != EINTR) {
- celix_logHelper_error(announcer->logHelper, "Announcer: Error
Selecting event, %d.", errno);
+ celix_logHelper_error(announcer->logHelper, "Announcer: Error
Selecting event, %d.", errno);
+ sleep(1);//avoid busy loop
Review Comment:
Concerning the `sleep(1)` / retry on a error rc. I think this is fine, but
currently the running is not updated in the else branch of `if (result > 0`)
and this is an issue.
I think the running update can be moved to the end part of the while loop:
```
while (running) {
....
celixThreadMutex_lock(&announcer->mutex);
running = announcer->running;
celixThreadMutex_unlock(&announcer->mutex);
}
```
ensuring that running is updated every loop.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@celix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
