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

java: rewrite support for AWS Java SDK v2 for newer versions #16

Closed nyh closed 7 months ago

nyh commented 7 months ago

As our java/README.md explained, our support for Java SDK v2 was complicated and ugly, and required replacing the Amazon's HTTP client implementation by our own modified fork. This forked version stopped working starting in Java SDK 2.20 - because of bad interaction with other SDK internals that kept changing.

In this patch we replace the complicated load balancer for Java SDK v2 by a much simpler and much shorter solution which uses the normal unmodified DynamoDBClient builder and doesn't require any forked HTTP client - it just requires passing an endpointProvider() option to the client builder.

The java/README.md, and the demo Demo2.java, is updated to use the new approach (and, again, everything is simpler and more natural).

Unfortunately, the new support does not work on SDK v2 versions prior to 2.20, as they didn't support the endpointProvider() feature. I believe that no users will have a compelling reason to stay on old versions of the SDK and not upgrade. If we ever have such a user that insists on an antique SDK, we can revive the implementation deleted in this patch as a third implementation (for pre-2.20 SDK v2). But I hope we'll never need to do that.

Fixes #15

nyh commented 7 months ago

Regretfully, it doesn't seem that anybody besides me is likely to review my PR (@roydahan in the long term we should consider moving this repository into the drivers team). So I'll probably merge it as-is soon myself unless someone is planning to review it and just didn't get around to it yet.

We usually don't allow people to commit their own patches, but that only works in a repository where there is more than one active contributor, and doesn't seem to work well in this alternator-load-balancing repository, so it seems like I might have to commit my own patch.

roydahan commented 7 months ago

@avelanarius can you please help reviewing?

nyh commented 7 months ago

I merged my PR myself.

elcallio commented 7 months ago

A bit late, but looks ok to me. :-)