tonyespy / gxds Goto Github PK

I think in terms of being idiomatic go, requiring a device-service implementation implement a method which specifically requests something happen asynchronously or in another goroutine isn't clean design. Typically if you want something to happen asynchronously or in another goroutine, rather than requesting the device-service start it's own goroutine, the caller would just call the normal function (i.e. the ProcessCommand method) in it's own goroutine. Then the implementers only need to implement a single function for processing commands and make sure that that function is thread-safe.

I think one argument for doing it the way currently implemented is that it directly tips off the implementer that their device-service needs to be thread-safe as the method they are implementing is called ProcessAsync. However I still think having a single method for processing commands and just inside gxds/service starting our own goroutine for doing this asynchronously is a cleaner API that results in less code duplication in the device-service implementations.

I will also point out that from my reading of the device-service requirements doc, I don't see any semantic differences between the async version and sync version of running a command, but perhaps there is and I didn't find it. If so, then that's another reason to keep it as is.

I'm not sure why the cache package needs the proto member here

If we remove this dependency as it doesn't seem to be used anywhere in the cache package we can move the ProtocolHandler interface into the gxds/service package where that interface is actually used.

Also I note that it may be necessary for some of the functions in objects.go (i.e. this comment), but I'm not familiar enough with the original codebase to say whether these functions need it or not...

All services (core and device) inherited from BaseService. So instead of duplicate config logic from core-services we can store it under /pkg dir in core and reuse here.