-
Notifications
You must be signed in to change notification settings - Fork 126
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
Discovery issues surfaced during device-rfid development #1001
Comments
This comment was written by @ajcasagrande in the afore-mentioned PR (which has already been merged). @tonyespy I can't comment on the re-factored version as I did not code-review it, but I can attest that the original code was not very pragmatic Go IMHO. As a developer I ran into many issues implementing the Discovery method due to the SDK (not just related to the locking), at which point I looked to the source to see why my code was not working as expected. From an outsider looking in, I was confused by the implementation as it did not seem to make sense.
|
I would guess that what we'd wish to do is something like:
Would this achieve what we need and can it be done without breaking? |
@ajcasagrande IMHOP we should've released device discovery under an experimental flag. It was developed as a stand-alone feature, when it would've made more sense to release it in conjunction with a device service which implemented discovery. Instead it was released without much actual testing, thus your team was really the first to ever use it, which meant you ran into many of the issues are described below... In the future, hopefully we'll abstain from releasing new SDK features in isolation like this.
Again, much of this falls out of poor requirements. The
No arguments... in theory, the serialization (if required) could've been handled solely via channels. That said, the fix now requires a wrapper function in order to handle the synchronization which is equally bad IMHOP.
The channel as defined by the Initialize() method is restricted to
Again, the
There's no reason discovery couldn't be a long running process similar to the way asynchronous readings are handled, however this wasn't part of the original requirements. |
This is what we have today (see comment below).
I don't see why the two are mutually exclusive as pointed out above, the
I think we should re-examine whether this serialization is actually needed? What are we trying to accomplish? AFAIK we don't have any other REST endpoints that we throttle like this. The implementation could choose to ignore subsequent calls to
|
Key thing here is to document what we have, eg
|
closing this as the doc elements are now covered elsewhere - see discussion on edgexfoundry/edgex-docs#598 |
version(s), 1.4, 2.0
This issue has been created as a result of a recent device-rfid-llrp-go PR which surfaced some issues with the existing discovery mechanism in the SDK, and some re-factoring which was done in response to issues raised by the device-rfid-llrp-go developers.
The following is taken from a comment I made in the PR reference above:
Finally, the locking issue (#609) is a bit puzzling. I believe the original design was correct and that releasing the lock when the SDK read a list of devices from the channel, was intentional. Discovery() was never meant to be a long-running operation that trickled up devices over a long period of time, it was intended to be called and result in a batch of discovered devices to be returned. That said, it clearly wasn't well-documented in the device-sdk-go's godoc for the ProtocolDiscovery.Discover() method:
// Discover triggers protocol specific device discovery, asynchronously
// writes the results to the channel which is passed to the implementation
// via ProtocolDriver.Initialize(). The results may be added to the device service
// based on a set of acceptance criteria (i.e. Provision Watchers).
One could argue that details about locking/synchronization in shouldn't be exposed in a public API, however there's no guidance to developers as to the fact that only a single instance of discovery can be running at a given time and that the
Discover()
method must write data to the channel in order signal completion of the operation. If this had been clearly communicated, there would've been no reason to re-factor the SDK to use a wrapper function to release the lock on completion of theDiscovery()
method. The original implementation was much cleaner/correct Go IMHOP.And finally, I'll mention that when the SDK was re-factored to "fix" this issue, the godoc still wasn't updated. I think it would be a good idea for us to do so... @iain-anderson @cloudxxx8
The text was updated successfully, but these errors were encountered: