rajveermalviya / unifrost

Making it easier to push pubsub events directly to the browser.
Apache License 2.0
169 stars 9 forks source link

aws sqs driver #3

Closed jbpratt closed 4 years ago

jbpratt commented 4 years ago

Good morning @rajveermalviya , This is a work in progress attempt at adding support for SQS and Azure service bus to Unifrost. SQS and SB seem to work quite differently (understandably) and I have no experience with either. Thoughts on what direction to go in would be great, you probably already have some thoughts for how you want this to be implemented.

rajveermalviya commented 4 years ago

Good Morning @jbpratt78 , Thank you taking the time in trying to understand the project, and contributing.

Adding support for Azure service bus & AWS sqs is surely important, the reason I didn't add it is because of my lack of experience with these services, I am more of a GCP user.

Azure Service Bus doesn't have a local emulator nor does it have a proper documentation, I presume only way to develop/test a driver for it would be using the cloud service. For AWS sqs/sns though you can first try creating a simple publisher and subscriber service and then look into developing the driver, you can use local emulators like localstack or goaws for developing locally without the AWS cloud service.

Currently topic subscriptions is handled by the Go CDK library for vendor neutral subscriber client, so using the AWS sdk directly would not be a great idea because it will break the compatibility as it will not implement the SubscriberClient interface in drivers/common.go. You can take a look at gcpdriver for reference drivers/gcpdriver/gcppubsub.go. So I would recommend you to use Go CDK library for developing the driver, though I have future plans for removing the dependency on Go CDK library.

Feel free to ask any other questions. Thanks again!

jbpratt commented 4 years ago

Good Morning @jbpratt78 , Thank you taking the time in trying to understand the project, and contributing.

Adding support for Azure service bus & AWS sqs is surely important, the reason I didn't add it is because of my lack of experience with these services, I am more of a GCP user.

Azure Service Bus doesn't have a local emulator nor does it have a proper documentation, I presume only way to develop/test a driver for it would be using the cloud service. For AWS sqs/sns though you can first try creating a simple publisher and subscriber service and then look into developing the driver, you can use local emulators like localstack or goaws for developing locally without the AWS cloud service.

Currently topic subscriptions is handled by the Go CDK library for vendor neutral subscriber client, so using the AWS sdk directly would not be a great idea because it will break the compatibility as it will not implement the SubscriberClient interface in drivers/common.go. You can take a look at gcpdriver for reference drivers/gcpdriver/gcppubsub.go. So I would recommend you to use Go CDK library for developing the driver, though I have future plans for removing the dependency on Go CDK library.

Feel free to ask any other questions. Thanks again!

Awesome, I think I understand a lot more now. I have updated the two drivers..

Neither the AWS or Azure Gocloud library provide a client like the GCP does. So I am just making use of both of them having OpenSubscriptionURL and utilizing the URLOpener. Using this allows for the end user to create this struct and add on options that are desired. There are some for Topics and Subscriptions on both awssnsqs and azuresb.

I am going to sit down and write out examples to test and prove the functionality. Thanks for being patient and working with me. Happy to make any changes.

rajveermalviya commented 4 years ago

URLOpener is great but it can be a problem when we migrate from Go CDK to using the specific vendor drivers, also in configuring when will provide a standalone binary. Can you migrate the current code to use the constructor instead of URLOpener. Check the gcppubsub implementation which uses constructor instead of URLOpener.

jbpratt commented 4 years ago

Using the URLOpener is easy but it can be a problem when we migrate from Go CDK to using the specific vendor drivers, also in configuring when will provide a standalone binary. Can you migrate the current code to use the constructor instead of URLOpener. Check the gcppubsub implementation which uses constructor instead of URLOpener.

Sure, the reason I didn't do this initially is because it requires unifrost to have "github.com/aws/aws-sdk-go/aws/session" as a dep. It seemed that the only thing that using URLOpener was allowing us to take in just the URLOpener from GoCloud (thus allowing the end user to configure it), rather than us taking in the client.ConfigProvider and the variadic SubscriptionOptions.

I did think these through but just decided on a direction to go.

With that said, should the session be attached to the client? Same for Azure's namespace. This will mean that Unifrost will depend on the AWS and Azure SDKs. Thoughts?

rajveermalviya commented 4 years ago

Depending on AWS and Azure SDKs is not a problem as Go CDK already depends on them internally. The reason we are using Go CDK is because it provides unified API for all the different vendors.

jbpratt commented 4 years ago

@rajveermalviya sorry this has ended up being a lengthy PR, thanks again.

Swapped the sqs driver to be how you requested and am stumped on how to do the Service Bus implementation since it takes in a few extra parameters that would break the interface implementation. Rather than using the constructor for Azure SB, should I just use this? Note, I have pasted in an example above the Azure SB Subscribe function for now as well as the constructor is right below the example linked.

rajveermalviya commented 4 years ago

Alright, I looked into Azure SB constructor in Go CDK you are right it will break the interface implementation. I would suggest you to to keep this PR for SQS only and probably deal with Azure SB later in another PR as it needs some more investigation and I am currently looking for tagging a release for unifrost. If you have any other suggestions please let me know.

rajveermalviya commented 4 years ago

Thank you @jbpratt78 for investing your time in contributing this PR.