[#1858] Skip removeCommandHandlingAdapterInstance if entry has expired

[calohmn]

This is for #1858 and updates the implementation based on the API spec change in PR #1917:

With the 'lifespan' parameter of the 'setCommandHandlingAdapterInstance' operation being mandatory to implement now for Device Connection API implementations, the explicit removal of a command handling adapter instance entry can be skipped if the entry has expired.

[sophokles73]

do you think it is actually worth the effort to do these checks? Why not simply send a remove message in any case?

[calohmn]

Effort-wise (from a runtime perspective), I think it's better to do this time-interval check here instead of sending the remove message for an entry that has already expired anyway.
This also means that there will be no "404 not found" exception triggered each time a remove message is sent for an already expired entry.

[sophokles73]

This also means that there will be no "404 not found" exception triggered each time a remove message is sent for an already expired entry.

The adapter doesn't care about the outcome anyway, does it? In the end, what is an adapter supposed to do if it gets a 404?

[calohmn]

The adapter doesn't care about the outcome anyway, does it? In the end, what is an adapter supposed to do if it gets a 404?

There is nothing actually done with the outcome, yes. But the 404 outcome will be visible in the tracing span - marking the whole span as an error (setting the error span tag is done in the AbstractRequestResponseClient base class method). And that should be avoided if the 404 error is triggered because the entry has expired, meaning there is no actual error at all.

In general I think having that method included in the overall span is important to see any other kinds of errors when they occur here. (I also have a PR in preparation letting the remove operation span be included in the parent unsubscribe operation trace.)
Therefore I would rather avoid these unnecessary 404 errors.

[sophokles73]

my understanding is that we return a 404 if the currently registered command handling adapter instance is not the one that has been passed in to the remove command, right? I do understand that we return a 404 in that case in order to let the client know, but I wouldn't consider this a reason to mark a Span as erroneous since it is an expected (and handled) situation because the device might have connected to another adapter instance already, or am I mistaken?

[calohmn]

I do understand that we return a 404 in that case in order to let the client know, but I wouldn't consider this a reason to mark a Span as erroneous since it is an expected (and handled) situation because the device might have connected to another adapter instance already, or am I mistaken?

Yes, a 404 shouldn't be regarded as an error in any case here.

In order to let the DeviceConnectionClient implementation use the base class mapResultAndFinishSpan() behaviour of marking a failed Future with an error span tag, I think it would be better to let the DeviceConnectionClient.removeCommandHandlingAdapterInstance method return a Future<Boolean> instead of Future<Void>.
This would mean a slight mismatch with the DeviceConnectionService server method result definition and the API definition (which we can't change in that way for Hono 1.3 of course), but I think that would be ok if documented properly in DeviceConnectionClient. WDYT?

[sophokles73]

Makes sense to me

[calohmn]

I've created #1926 to replace this PR, including the above mentioned changes to the DeviceConnectionClient.