David Harkness

Not necessarily. I'm talking about refactoring the Mappings class which wouldn't need an interface, but you were speaking about ClientData, right? I'm not sure what part of the code you meant. What responsibilities do you want to refactor into separate classes? Start by listing those, and some appropriate class names will pop out at you.

Any time you see fields with similar names like primaryHostIdToPartitionMapping and secondaryHostIdToPartitionMapping it's a clue to a refactoring. Both mappings require hostIdToMachineMapping which could be shared easily between two mapping class instances. That's an example of what I mentioned earlier.

There's no reason they couldn't share an instance to manage the block list and others to manage the host-to-IP mappings

@lining That sounds similar to what I was thinking from your description, though I would say two instances of the same class (maybe what you meant).

you're welcome. good luck!

ok I'll be in and out today but I'll keep the window open. that was a fun exercise :)

cool

starting to look like JavaScript :p

added signaling ultimate failure when you run out of hosts or all fail

ok, got it

the problem is that the callback needs to continue/break, but you don't want to duplicate the code. lemme think a min on it...

it's complicated and still needs to handle the looping correctly. i.e. it's not done.

ok I updated it

problem is that then the requests needs to block on the future, requiring a new thread. It could use an anonymous class as the callback inside execute.

right now the callback sets the final result on the SettableFuture when successful. oh, hmm, inversion of control here. Ideally you'd set a future for success failure on the request's future--not the final future. then the request would break/continue depending on success/failure...

filled in a little more

gist.github.com/dharkness/ca829f9ade2a0d854b0d

the failover is a little tricky. probably need to move the success/failure into the new DataRequest class with its own future.

sure, gimme a min

so DataClient only uses the key and ip mappings to build a list of hosts and create a DataFetchRequest.

a DataFetchRequest

or encapsulate the hosts and other necessary data into a new class that gets created by getData() and is responsible for executing the calls to the server and failover.

so ideally you would refactor that part of the code from getData() and have onFailure() call into it

yeah, you answered as I asked

haha, jinx

I see you're passing the remaining IPs to the callable, I assume for failover. Does it end up doing the same logic to check for blocked hosts?

This avoids the iterator entirely

while (!listOfHostnames.isEmpty()) {
    String hostname = listOfHostNames.remove();
    if (!ClientUtils.isEmpty(hostname) && !ClientData.isBlockHost(hostname)) {
        executeForServers(responseFuture, key, firstHostname, listOfHostnames);
    }
    isFound = true;
    break;
}

oh you mean because you're modifying the list while iterating? possibly, tho the iterator may be smart enough to avoid it. You could change the code to avoid it for sure:

And that could be made simpler by giving the list of blocked hosts to the class that maps host names to IP addresses to return null for any blocked host straight away

You could move the logic for checking the blocklist into the code that produces the list of hostnames, omitting any host that is already blocked

lemme check

Cool. I built that backend two years ago and haven't had to touch it since. No wait, three years? wow

I haven't used AsyncRestTemplate yet (Spring 4.0?), but it looks handy. I have a backend service that uses futures for the results but threads for the REST calls that produce their results. It would be better to have it all be futures.

Okay, the logic looks clear enough. I'd definitely refactor the longer methods into shorter ones which will help point out where you could extract logic into classes. But it works so... :)

ok reading that...

Q: You look up pri/sec loc/rem hostnames every time. Do you use all four IPs every time or only some of them based on the values in the DataKey?

Another principle: "Tell, Don't Ask".

Better still frank.driveCarIfBlue()

Better to do if (frank.hasBlueCar()) { ... } :)

Asking a different class ClientUtils to inspect mappings from another class is dodgy.

Yes

It may not seem like much now because of all those helper methods. If you refactor the maps into classes it becomes cleaner.

public boolean hasPrimaryHostMappings() { return !primaryHostIdToPartitionMapping.isEmpty(); }

and in Mappings class:

if (mappings.hasPrimaryHostMappings()) {...}

ok let's hit that first.

Can you show me the code that uses those four IP addresses? I suspect if you were to encapsulate the maps you could simplify that logic.

ok