scylladb / alternator-load-balancing

Various tricks, scripts, and libraries, for load balancing multiple Alternator nodes
Apache License 2.0
18 stars 11 forks source link

Upgrade to aws-sdk-go-v2 #13

Closed jangraefen closed 1 year ago

jangraefen commented 1 year ago

Any feedback is welcome 🙂

mykaul commented 1 year ago

@nyh - can you or someone else review this?

jangraefen commented 1 year ago

Any update on this? 🙂

nuivall commented 1 year ago

Thank you for the contribution! It looks good to me!

I left some minor comments.

jangraefen commented 1 year ago

I have a hard time reviewing this because I never used our Go load balancer library or even the AWS Go library (I usually use the Python version for my own tests). There's something that bothers me: If we already have working code for aws-sdk-go v1, do we really want to drop it completely and switch to having only aws-sdk-go v2? Can't we have both, perhaps having a section in the README about v1 and v2 support, and perhaps two different alternator_lb.go files if the code is different? I agree it's ugly and hard to check and maintain both versions, but since presumably the v1 code is already working, and now you're adding v2 support, so I wonder if we can't keep both at least for now. Or, maybe you can tell me that nobody uses the v1 sdk any more so it's safe to drop support for it and forget all about it?

No, you are completly right. There are still a lot of people who rely on the AWS SDK v1 for Go. I will move the new code to a go-v2 folder and add back the old code as go-v1. I left the code as is thoug. If you want, I could try to add my improvements from go-v1 here as well.

nuivall commented 1 year ago

Or, maybe you can tell me that nobody uses the v1 sdk any more so it's safe to drop support for it and forget all about it?

Note that the way v1 was written (which @jangraefen rightfully corrected in v2) made it quite difficult if not impossible to use the code as a library, for instance

nyh commented 1 year ago

Or, maybe you can tell me that nobody uses the v1 sdk any more so it's safe to drop support for it and forget all about it?

Note that the way v1 was written (which @jangraefen rightfully corrected in v2) made it quite difficult if not impossible to use the code as a library, for instance

* lack of module support means users need to trick their setup to include such code as a library (most will probably just plain copy the code)

* `alternator_nodes.session` - small letter here means the function is unexported, so nobody could have used it from a different package (directory)

It sounds like these things can be easily fixed by someone who cares about SDK v1. I guess until now nobody really cared :-(