[#1858] Add lifespan to setCommandHandlingAdapterInstance #1908
Conversation
calohmn
added
C&C
calohmn
force-pushed
the
PR/devcon_add_lifespan
branch
from
53ed37c to
d9ffbe4
Compare
|
|
||
| private static final Logger log = LoggerFactory.getLogger(MapBasedDeviceConnectionService.class); | ||
|
|
||
| // <tenantId, <deviceId, lastKnownGatewayJson>> | ||
| private final Map<String, Map<String, JsonObject>> lastKnownGatewaysMap = new HashMap<>(); | ||
|
|
||
| // <tenantId, <deviceId, adapterInstanceIdJson>> | ||
| private final Map<String, Map<String, JsonObject>> commandHandlingAdapterInstancesMap = new HashMap<>(); | ||
| private final Map<String, Map<String, ExpiringValue<JsonObject>>> commandHandlingAdapterInstancesMap = new HashMap<>(); |
There was a problem hiding this comment.
it might be easier to use a Caffeine Cache instead which already supports limiting size and defining a time-based eviction strategy ...
There was a problem hiding this comment.
I've committed a change so that a Caffeine Cache is now used, letting it do the eviction.
I've haven't used the size limited feature though (maximumSize on cache builder) because that would change the behaviour. When the max size is (about to get) reached on a Caffeine Cache, some existing entry gets evicted. What we want here is to keep the existing entries and prevent addition of new entries if the max size has been reached.
calohmn
force-pushed
the
PR/devcon_add_lifespan
branch
from
d9ffbe4 to
796c543
Compare
| @@ -126,6 +126,7 @@ The following table provides an overview of the properties a client needs to set | |||
| | :-------------------- | :-------: | :----------------------- | :-------- | :---------- | | |||
| | *subject* | yes | *properties* | *string* | MUST be set to `set-cmd-handling-adapter-instance`. | | |||
| | *adapter_instance_id* | yes | *application-properties* | *string* | The identifier of the protocol adapter instance that currently handles commands for the device or gateway identified by the *device_id* property. | | |||
| | *lifespan* | no | *application-properties* | *int* | The lifetime of the mapping entry in seconds. After that period, the mapping entry shall be treated as non-existent by the *Device Registration API* methods. A negative value, as well as an omitted property, is interpreted as an unlimited lifespan.| | |||
There was a problem hiding this comment.
what if the client provides the value but the implementation doesn't support it?
There was a problem hiding this comment.
I've adapted the description as follows:
The desired lifespan of the mapping entry in seconds. If the Device Registration API implementation supports this property, the mapping entry shall be treated as non-existent after that period has elapsed. A negative value, as well as an omitted property, is interpreted as an unlimited lifespan. A Device Registration API implementation may not support this property, meaning that a client can't rely on the given lifespan to be applied.
There was a problem hiding this comment.
@sophokles73 An afterthought on this:
Since we target this change for the next minor version, meaning we can introduce incompatible changes to providers of the Device Connection API, I think we can indeed require implementations of the API to support the lifespan property. And I think we should make it a requirement then (otherwise we have to maintain extra client logic for the case it isn't supported by an implementation). WDYT?
There was a problem hiding this comment.
How could a client know if an implementation supports it or not? The implementation would simply ignore the given value and keep the mapping until it evicts it, wouldn't it?
There was a problem hiding this comment.
My point is to make it mandatory for an implementation to support the lifespan property, so that a client can be sure it is supported.
There was a problem hiding this comment.
Since we target this change for the next minor version, meaning we can introduce incompatible changes to providers of the Device Connection API, I think we can indeed require implementations of the API to support the lifespan property.
I am not sure about this, because the (public) Device Connection API describes a dependency on an external system as opposed to APIs where Hono is the actual implementation (provider) of the API, e.g. the API defined by its protocol adapters towards the devices.
There was a problem hiding this comment.
Created PR #1917 for adapting the documentation.
EDIT: sorry, didn't see the above comment, waiting on further comments here then.
There was a problem hiding this comment.
I am not sure about this, because the (public) Device Connection API describes a dependency on an external system as opposed to APIs where Hono is the actual implementation (provider) of the API, e.g. the API defined by its protocol adapters towards the devices.
I was sticking to the OSGi semantic versioning notions of providers and consumers (with the emphasis of keeping only consumers stable between minor versions) here and seeing the device registry as a provider of the different Hono APIs.
But, yes, in the sense that the device registry is external, it could be seen as even more important to keep things compatible for it between Hono minor versions than for the protocol adapter consumers, which are Hono internal (in a way).
But I think, if we go that way of keeping device registry implementations compatible between Hono minor versions, we have to increase Hono major versions in more cases. For example, the Command & Control changes in 1.2 (with its additional, non-optional Device Connection API methods) would have required a 2.0.
And now, the addition of another additional API flag, mentioned in #1858 (comment), where the support in the implementation should better be not optional, would strictly speaking require us to go for 2.0 instead of 1.3 as well.
In any case, we should decide on which approach to take.
There was a problem hiding this comment.
The registry APIs defined as part of Hono all define APIs that Hono consumes, i.e. they define what Hono expects to find implemented by some third party entity. The fact that we also provide an example implementation of these APIs doesn't mean that we can change these APIs in a non-backward compatible way as long as we implement these changes in the example registry. So, indeed, I agree that we already would have needed to increase the version to 2.0.0 instead of 1.2.0 because of the newly introduced, mandatory operation in the Device Connection API.
There was a problem hiding this comment.
On the community call last Thursday it was decided that we want to stick to the OSGi semantic versioning approach.
An implementation of the device registry APIs is seen as a provider. Hence there can be incompatible changes between Hono minor versions for the providers, requiring changes to the implementation of the device registry APIs. But, at the same time, these API changes must keep the consumers, i.e. the protocol adapter implementations, compatible with the previous minor version.
That means that a device registry implementation of a newer minor version still is compatible with protocol adapters of an older minor version of the Hono APIs.
This was seen as the key point here.
It is the obligation of the device registry implementation, to keep up with additive changes in newer minor versions of the APIs then.
This makes it possible to restrict the lifespan of entries set via the setCommandHandlingAdapterInstance method of the Device Connection API. Signed-off-by: Carsten Lohmann <[email protected]>
calohmn
force-pushed
the
PR/devcon_add_lifespan
branch
from
796c543 to
c2a9039
Compare
This is for #1858:
This makes it possible to restrict the lifespan of entries set via the
setCommandHandlingAdapterInstancemethod of the Device Connection API.Actual usage of the new parameter will be added in a subsequent PR.