Add rack/dc aware load balancing

[dkropachev]

Closes https://github.com/scylladb/alternator-load-balancing/issues/35

Core PR that introduced rack and dc filtering on /localnodes: https://github.com/scylladb/scylladb/issues/12147

This PR allows user to target dynamodb client to a particular rack/datacenter or rack+datacenter, via following API:

        AlternatorRequestHandler handler = new AlternatorRequestHandler(uri, datacenter, rack);

Changes in auxiliary AlternatorLiveNodes API:

1. Constructor input argument is an single URI now
2. Validation is introduced
3. Start is moved out from the constructor

       liveNodes = new AlternatorLiveNodes(Collections.singletonList(seedURI), datacenter, rack);
       try {
           liveNodes.validate();
           liveNodes.checkIfRackAndDatacenterSetCorrectly();
       } catch (AlternatorLiveNodes.ValidationError | AlternatorLiveNodes.FailedToCheck e) {
           throw new RuntimeException(e);
       }
       liveNodes.start();

Tested

1. Regular, old code: AlternatorRequestHandler handler = new AlternatorRequestHandler(uri) for 6.2.0 and master
2. Rack-aware: AlternatorRequestHandler handler = new AlternatorRequestHandler(uri, "dc1", "rack1") for 6.2.0 and master
3. Dc-aware: AlternatorRequestHandler handler = new AlternatorRequestHandler(uri, "dc1", "") for 6.2.0 and master

All tests are done for following deployments:

1. MultiDc
2. MultiRack+MultiDc
3. SingleDc

[mykaul]

@nyh please take a look.

[nyh]

If I understand correctly, the example you gave and the Demo1 you modified are for the old Java SDK (v1). Did you also check the newer SDK v2 (it was released 7 years ago - it isn't very new...)? If you didn't, please do. If you did, please mention that both are supported (or not) in the commit message.

[dkropachev]

Thanks, looks mostly good for SDK v1 (I think it breaks v2?), but I think but I have one objection about the need for a new start() which needlessly (I think) changes the API of this library. Wouldn't it be easier if we just had a constructor that takes in addition to a url, also dc and rack? Basically, exactly the same code you added to Demo1 (the constructor will take both dc and rack, but null means not to set one or both).

I consider having thread running when you initialize class as bad practicce that does not allow properly test it and makes code hard to read and understand: In this particular case if you did not look at constructor you would not even know that there is a thread running underneeth.

I am also not sure there is a need for all the complexity you added for checking if the rack/dc feature is supported, and retesting this every minute. After all, the rack/dc option is something the user explicitly chooses for a specific cluster - just like the initial address of one known node. Why would a user pass wrong parameters? Why does the library need to check and recheck it, instead of just saying - if you pass wrong parameters, you'll get wrong results?

We had exactly same problem in drivers, user provides wrong rack, datacenter or something and can't figure out why queries are failing. With this code they have opportunity to explicitly check if they match.

Regarding check if feature is supported or not. same reason, if we can tell user that target cluster does not support feature, we should do that, otherwise code will produce unexpected results and user left to guess what is going on.

I don't mind if we have a method like checkSupportForRackandDC() or something like that, maybe some user will find it convenient, but this can be a stand-alone function and doesn't need to be mixed into all the other functions.

Makes sense, let's do it this way.

[nyh]

Thanks, looks mostly good for SDK v1 (I think it breaks v2?), but I think but I have one objection about the need for a new start() which needlessly (I think) changes the API of this library. Wouldn't it be easier if we just had a constructor that takes in addition to a url, also dc and rack? Basically, exactly the same code you added to Demo1 (the constructor will take both dc and rack, but null means not to set one or both).

I consider having thread running when you initialize class as bad practicce that does not allow properly test it and makes code hard to read and understand: In this particular case if you did not look at constructor you would not even know that there is a thread running underneeth.

I don't why the user needs to care if the object contains a thread or not, but if you decided to change this part (and you didn't strictly have to do this, you could have added a constructor parameter), you'll need to change the documentation - and hope nobody is using the existing version and will complain about the API change :-) Also I think the SDK v2 also uses this class so will need to change too.

I am also not sure there is a need for all the complexity you added for checking if the rack/dc feature is supported, and retesting this every minute. After all, the rack/dc option is something the user explicitly chooses for a specific cluster - just like the initial address of one known node. Why would a user pass wrong parameters? Why does the library need to check and recheck it, instead of just saying - if you pass wrong parameters, you'll get wrong results?

We had exactly same problem in drivers, user provides wrong rack, datacenter or something and can't figure out why queries are failing. With this code they have opportunity to explicitly check if they match. Regarding check if feature is supported or not. same reason, if we can tell user that target cluster does not support feature, we should do that, otherwise code will produce unexpected results and user left to guess what is going on.

So the setrack() function (or the constructor, in the old style) can do the check. Why would a user even think to call a separate function to check this? And why does this feature need to be re-checked every minute - are we worried that the user will downgrade the cluster and lose this feature (I don't even think this is possible in Scylla)?

[dkropachev]

@nyh , @Bouncheck , let's split it into parts, i will create smaller PRs to address separate issues and then we proceed with this one.

[nyh]

@nyh , @Bouncheck , let's split it into parts, i will create smaller PRs to address separate issues and then we proceed with this one.

Yes, although note that the rack-aware load balancing is the most important part, I think it should be done first before all the other minor (?) improvements. Also, I asked why you decided to fix this only for the v1 version of the SDK and not v2, and didn't get a reply.

[dkropachev]

@nyh , @Bouncheck , let's split it into parts, i will create smaller PRs to address separate issues and then we proceed with this one.

Yes, although note that the rack-aware load balancing is the most important part, I think it should be done first before all the other minor (?) improvements. Also, I asked why you decided to fix this only for the v1 version of the SDK and not v2, and didn't get a reply.

@nyh, @Bouncheck , could you please take a look agian, I have addressed comments and rebased to the master, also I have fixed both Demo1, Demo2 and Demo3. Covering aws sdk v2.

[nyh]

@dkropachev in a single-patch pull-request, it is the single patch's commit message - NOT the cover letter - which gets inserted into the git history and there will be no merge and therefore no cover letter. So please make sure that this single patch's commit message has the full description - and not just a single line.

[nyh]

@dkropachev in a single-patch pull-request, it is the single patch's commit message - NOT the cover letter - which gets inserted into the git history and there will be no merge and therefore no cover letter. So please make sure that this single patch's commit message has the full description - and not just a single line.

Oh, and of course please ensure that the commit message is up-to-date with the code. I see for example that instead of the setrack() et al. that you described, now it's part of the constructor.

[dkropachev]

@dkropachev in a single-patch pull-request, it is the single patch's commit message - NOT the cover letter - which gets inserted into the git history and there will be no merge and therefore no cover letter. So please make sure that this single patch's commit message has the full description - and not just a single line.

Oh, and of course please ensure that the commit message is up-to-date with the code. I see for example that instead of the setrack() et al. that you described, now it's part of the constructor.

Done, please take a look at commit and PR description.

[roydahan]

@nyh can you please prioritize this review so we can "release" it?

[nyh]

@nyh can you please prioritize this review so we can "release" it?

Will review it now.

[dkropachev]

@nyh, there was an issue with github UI not picking up new commits from the branch, I managed to fix it by pushing whole branch, as it is to the origin. Unfortunately, I did not spot it in time and last review your did was done on an outdated version. I am sorry for it to happen, but I have to ask you to rereview it again.