Yes, you're on exactly the right track (what I was about to suggest after reading halfway through the code). As long as all the logic goes into the Mappings class and you call getMappings() once and reuse the same mappings local variable, all calls will access the same trio of maps that were set together.

In other words, this is thread-safe and will work as you want.

Be sure to move all logic that externally accesses those three maps inside the Mappings class. For example, if (!ClientUtils.isEmpty(mappings.primary)) becomes if (mapings.hasPrimaryMappings()). But further, perhaps all the logic inside the block for this if can be moved into Mappings.

And as more logic moves in there, Mappings should probably a) be promoted to a regular (non-static) class and b) be given a more descriptive name.

Seeing how those two if blocks are identical except for primary vs. secondary, you may even want to split Mappings into several classes where each map gets moved into a Mapping class. They can still be created together to protect the concurrency, but the shared logic can be moved into a single class that is instantiated for each map. I'd need to see the rest of the logic to see if it's worth it, but it seems so.

I'll keep this chat room open for a while so feel free to respond.

@DavidHarkness hey Thanks for joining in. I see so looks like I was on the right track and I was in the impression may be with the new code I have, I will not access same three maps together but I was wrong.

If you are around, I can show you how my original client data class looks like and how it is being used as of now and how we are setting the three maps from the background thread

This is my original "ClientData" class as shown in this Gist -

https://gist.github.com/raihan26/8cb9fc0e3dbb86a55f9d

And this is my background thread class which runs every two minutes and will create the mappings and set it in ClientData class. If you remember update on three maps will happen once in 5-6 months.

gist.github.com/raihan26/b1b7581f8488f16227ac

And this is my original DataTaskExecutor class where I am using the Mappings class by extracting the three maps for each DataFlowEnum type

gist.github.com/raihan26/f4e8766d02bd9e2fb867

So these are my old setup as of now. Sorry for posting everything in one shot, it might be causing confusion to you. I will be around today as I am working on some stuff, if you will be around for 15-20 minutes, I can explain you if you are confuse.

@lining Okay, checking them out now. Think of each Mappings instance as providing a single snapshot-in-time of those three maps and logic to act upon them.

hey

sure. Let me know if anything is unclear.

Also, I'm always wary of static fields and classes. Are you running in a web/EE container? Are you using Spring or some other container framework for wiring components? Spring provides facilities for ensuring singletons as well as component initialization ordering that would allow removing the countdown latch. Another option is to initialize the class with three empty maps if that won't break the logic.

No, this is actually a simple Java library by using maven as the dependency

and other people will just my maven version of library in their pom

I am using HttpClient provided by Spring which is RestTemplate

In general logic is like this, Customer will use this library and they will pass DataKey object which will have userId and other fields in it. Basis on userId, I will get the partitionId and then basis on partitionId, I need to find out what the hostnames I can execute by making the url.

If you're building a library, it's even more important to avoid statics. Expose a class the client can instantiate and manage on their own, either through Spring or a static field if they want to go that route. Using statics internally forces a design decision on them.

I see

These ClientData and Mappings class are being used by the internals of the library to decide what are the hostnames given a userId.

People will pass DataKey builder object which will have userId in it and then I need to return a DataResponse object which will have the response in it.

Is there any chance a client might need multiple sets of Mappings for different purposes at once? If so, your library blocks that option by using a single static value.

It also makes testing harder. There are many reasons to avoid statics and many ways to work around them easily. I suggest a little Googling on the pitfalls of static code.

But for the question at hand (moving logic into Mappings and ClientData), things like this look suspect:

if (!ClientUtils.isEmpty(responses)) {
    ClientData.replaceBlockedHosts(blockList);
}

The response is the responsibility (no pun intended) of the thread which includes extracting a block list from it. It should not then pass the raw response to ClientData to have it decide if it wants a block list. Either the thread should determine this or ClientData should use the block list to decide.

Try to follow the Single Responsibility Principle to simplify your code and make it more testable.

You are worried about the if check mainly? That was added so that I don't override the maps if the responses map is empty because the server was down. And because of that reason I added that if check

It is not passing raw response, if you see it is passsing

blockList map which will have the hostname to block it

The logic is sound, but having ClientUtils inspect the HTTP response is the wrong responsibility.

I see what you mean

It is passing responses to ClientUtils to do the check.

Can this be if (!blockList.isEmpty())?

Probably not if the server may return a valid empty block list.

Ok setup is like this

If that's the case, have the thread class contain the logic to inspect the response.

sure got it now

I have a system in which userId is sharded across our datacenters. We have thee datacenters, and in each datacenters we have 11 machines. Each machine is responsible for certain set of user id so those three maps will tell me which machine to go and get the data.

So setup will remain same for atleast 5-6 months but let;s say if we add more machines in the datacenter, then their will be shuffling of user id's

and then only the partition mapping will change

Where is the logic to map from userId to machines?

and then this will be called

That makes sense.

if (update) {
	ClientData.setAllMappings(partitionMapper);
}

to update the mappings

those three maps contains the details to extract the hostnames given a user id

My background thread makes a call to my service which returns back the mapping of partitionId to machineName.

Meaning if customer is passing DataKey object in which user id is 1234, then I will do this calculation

One thing that concerns me: the Mappings class ensures that the three maps are set and used together, but the thread makes four calls to a remote service to get the data to build the maps and block list.

four calls?

or it is two calls right?

What values does DataKey contain? userId and partitionId?

No it will have userId

and other fields like timeout value

one call per enum value (aren't there three values?) and another call for the blocklist. Or maybe I misread the code.

so basis on userId, I will get the partitionId

Ah ok

using this formula

userId -> partitionId

userId Mod 1680

will give me partition id

basically our system is sharded on userid

using 1680 partitions

So the client can make requests for many user IDs using the same set of maps. All good.

so any userId comes

we will do userId mode 1680

to get the partitionId

and then basis on partitionId we extract

What are the values of DataFlowEnum?

now you will be asking how we handlle fail over cases

Those are two different cases. Like some customer will pass userId as the key and some will pass another key like cguid

so if they are passing userId, I will be using USERFLOWENUM

and they are passing CGUID, I will use DEVICEFLOWENUM

and the mappings is different for them

so that's why I do it like this

// here keys.getgetFlowType() will return me which DataFlowEnum mappings
	// I want. If you see closely I am creating three maps for each DataFlowEnum type
	Mappings mappings = ClientData.getMappings(keys.getFlowType());

if you see it closely

so that I can get the mapping for the flowtype they are asking. If they are asking me userId, then I will get the hostnames from that mappings

Actually mappings is different for USERID and CGUID cases

and different set of machines as well

And it's okay that the mappings for each flow type may not line up? Meaning you make one remote call for USERFLOWENUM and get mappings, then the remote mappings change, then you make the second call for DEVICEFLOWENUM and get a new set of mappings. This is okay?

they will not pass both the keys at the same time

they will pass either USERID

Okay, so when mappings change for USERID because a new machine was added, it doesn't affect the mappings for CGUID?

or CGUID

yes

No, your thread will make separate remote calls.

it will not affect mappings for CGUID

No due to the client making requests, just when polling for new mappings.

ok

Yeah in the same background thread I am just collecting the mappings

for those two flow type

and if you are saying that if USERID mappings changes

and CGUID mappings doesnt change, then still it will update the maps

right?

bcoz it is one single call

Then perhaps your should have two threads--one for each flow type--and simplify the logic. Each thread will work for a single flow type. It seems like this would be simpler, but hard to be sure without knowing all the code.

yeah I could do that as well but then I will have two AtomicReference right?

one for each mappings

It's okay if one set of maps changes and the other does. No biggie. I just wanted to make sure that when a new machine comes online and is added to the remote server that manages the sharding, it's okay if the machine appears in one set of mappings (CGUID) but not the other (USERID)

Thing is we have different production machines to deal with USERID case and different production machines to deal with CGUID case

if we add machines in USERID based servers, then only the mapping will change in the USERIDFLOWENUM

not in the CGUID one

Yes, but you could replace it with a HashMap<FlowEnum, AtomicReference<Map<String, Map<Integer,String>>>. LOL, say that three times fast!

Probably better to just use two AtomicReferences since the number of flow types is baked into the code.

sure

I am not mainly worried about this case bcoz it is like it might happen once in 6 months and after that it will stay the same. But I can try making it more simple

Okay, so multiple remote calls are safe. Separating the polling threads would also allow different polling intervals or push notification if you want to get tricky. Not necessary though, but I think it would clean up some of the logic. Everything having to do with looping over DataFlowEnum would be moved into a single place outside the thread.

Yes, I focus first on code complexity and worry about nickel-and-dime performance later.

yeah exactly we could do that for sure.

We are running this setup since more than an year now

and we haven't added any servers yet

:)

so that's why I was not worried at all for now

You can spend $100 and improve performance across the board. Better to save 10k on developer time trying to understand and maintain complex code.

lol makes sense :)

Yeah, YAGNI :)

so now coming back to modified ClientData class

gist.github.com/raihan26/d3ea4c6a267eeecdccf8

this is what you meant earlier?

So what question/concern do you have with the code now? I could throw out refactoring ideas all day, but if you don't need 'em...

main question was basically does my above ClientData class will be thread safe?

Yes for the code in Mappings class.

As long as the client calls getMappings() once and makes all those other calls on the same instance, it's thread safe.

        int partitionId = getPartition();

        Mappings mappings = ClientData.getMappings(keys.whichFlow());

        String localPrimaryHostIPAdress = mappings.getLocalPrimaryHostIPAdress(localZookPath, partitionId);
        String remotePrimaryHostIPAdress = mappings.getRemotePrimaryHostIPAdress(remoteZookPath, partitionId);
        String localSecondaryHostIPAdress = mappings.getLocalSecondaryHostIPAdress(localZookPath, partitionId);
        String remoteSecondaryHostIPAdress = mappings.getRemoteSecondaryHostIPAdress(remoteZookPath, partitionId);

and then I will use like this in the call method of my executor class

I would make version final as well. Any time you can make an instance field final, do it.

correct

sure

you were also suggesting me (mapings.hasPrimaryMappings())

Now, can you move the logic that uses those local/remote primary/secondary IP addresses into Mappings as well?

If so, you could drop the helper methods.

basically for this chheck if (!ClientUtils.isEmpty(mappings.primaryHostIdToPartitionMapping)) {

which helper methods?

getLocalPrimaryHostIPAdress() et al

Right now, Mappings holds these mapping tables which the caller uses to map hosts to IP addresses and then does something with them. What does it do with those IP addresses?

I will make an HTTP Call to them by making the URL

basically one importtant thing I didn't mentioned

Okay, so not the responsibility of Mappings. nevermind

Each user id data is replicated in two machines in each datacenter

for failover?

so let's say if frist machine is down and not responding, I will make a call to second hostname

yeah

and same thing with third and fourth

is that primary/secondary distinction?

yeah

what is local/remote distinction?

basically

when customer calls, we know from which datacenter they are calling

if they are calling from PHX, we will get the machines from local PHX datacenter

if they have secondary flag on, then I will get local and secondary machines in PHX datacenter

which is in PHX

and if they have remote flag on

then we will pick random datacenter as well which can be SLC or LVS

ok

and then we will pick two machines from either SLC or LVS

so that if local machines are down for whatever reason

we can go cronss datacenters

so this is upto customers what flag they are setting in DataKey object

How do I add this logic then

if (!ClientUtils.isEmpty(mappings.primaryHostIdToPartitionMapping)) {

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 let's hit that first.

yeah

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

and in Mappings class:

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

I see and same thing with other two maps as well

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

Yes

I see

that's what I was also going to ask

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

Yeah I need to get rid of this habits

I am doing this all over the places

I will definitely try improving on this

how you were thinking to refactor them in different classes?

lol

got it

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

yeah very well understood now

Better still frank.driveCarIfBlue()

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

yeah most of the times I always use "is" where I should be using has

got it on that as well

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?

If call is coming from PHX and secondary flag is enabled only and remote flag is disabled

then I will have two hostnames in the LinkedList

one is localPrimary in PHX

localSecondary in PHX

but let''s say if they also have remoteFlag enabled as well

then I will pick random datacenter leaving PHX, which can be SLC or LVS

so let's say I pick SLC

then I will have remotePrimary and remoteSecondary

so code is like thhis

this is my main class -

gist.github.com/raihan26/ab655f33956f9bf8deef

Customer will call getData method by using my factory

and they will pass DataKey object

which will have userId or cguid, remoteFlag option secondaryFlag option

and other stuff

I am using exchange method of AsyncRestTemplate which returns back a ListenableFuture on which I have added a callback

so that I can get notified whether response was successful or not

ok reading that...

and this is my RetryCallBack class

gist.github.com/raihan26/badcdbe3aacaa71b75e9

and that's it

so basically all the main logic is part of ClientData and Mapping class to tell me the hostnames given a partition id and localdatacenter and remotedatacenter information.

And this isi my interface for DataFetcher class

gist.github.com/raihan26/fed7cfd272bcbfc76ee9

Basically I am using AsyncRestTemplate so that I can have NIO based client connections

and get rid of ExecutorService thread pooling which I was using earlier

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... :)

sure

also logic of executing the hostnames is like this

- Given a userId, I will find out what are the machines that I can hit to get the data and then store those machines in a LinkedList so that I can execute them sequentially.
- After that I will check whether the first hostname is in block list or not. If it is not there in the block list, then I will make a URL with the first hostname in the list and execute it and if the response is successful then return back the response. But let's say if that first hostname is in the block list, then I will try to get the second hostname in the list and make the url and execute it so basically find the …

This is the main logic on which our system worked.

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.

yeah it is 4.1.6

As of now I have duplicated code around the above four logic

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

cool

sounds pretty amazing

If you see my getData method and then onFailure method

code is duplicated

do you see is there any way we can simplify that

basically this part

for (String hostname : listOfHostnames) {

in getData method

lemme check

this revolves around the above four points

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

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

but that list keeps getting updated every 2 minutes

also is there any bug which can throw ConcurrentModificationException as well?

in the getData method?

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:

boolean isFound = false;
for (String hostname : listOfHostnames) {
	// check to see whether hostname is blocked or not
	if (!ClientUtils.isEmpty(hostname) && !ClientData.isBlockHost(hostname)) {
		String firstHostname = listOfHostnames.removeFirst();
		executeForServers(responseFuture, key, firstHostname, listOfHostnames);
		isFound = true;
		break;
	} else {
		listOfHostnames.removeFirst();
	}
}
// all the servers were in block list
if (!isFound) {
	responseFuture.set(new DataResponse(null, DataErrorEnum.SERVICE_UNAVAILABLE,

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

I don't think so

This avoids the iterator entirely

Aahhh

cool

I see

now the same check I am doing it as well in onFailure method

as well

This is why I said I have code duplicated around

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?

bcoz I am calling recursively executeForServers

method

haha, jinx

yeah :(

yeah, you answered as I asked

is there any way to simplify that?

see this

@Override
public void onFailure(final Throwable ex) {
    if (ex instanceof SocketException) {
        // if it comes here, then it means some of the servers are down so adding it into block list
        ClientData.blockHost(hostname);
        for (String host : restHostnames) {
            if (!ClientUtils.isEmpty(host) && !ClientData.isBlockHost(host)) { // check to see whether hostname is blocked or not
                String nextHostname = restHostnames.removeFirst();
                dataFetcher.executeForServers(responseFuture, beKey, nextHostname, restHostnames);

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

but the firsthostname which I am passing to executeForServers

has to be checked as well for block hostname list

as well

SO I was thinking may I should have all these logics

in executeForServers method itself

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.

a DataFetchRequest

hmmm

Can you show me how can I do that as an example?

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

sure, gimme a min

ohkk

I added this interface gist.github.com/raihan26/fed7cfd272bcbfc76ee9 so that I can call it recursively

on the fail over cases

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

this was looking tricky to me and I end up duplicating the code

doing same stuff at both the places

gist.github.com/dharkness/ca829f9ade2a0d854b0d

how do we know whether it is successfull response since I was adding a callback on the future

filled in a little more

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...

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.

In this case, do I need to block the future to get the response?

and then decide whether it was successful?

ok I updated it

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

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...

yeah that's why this is tricky

ok, got it

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

yeah looks good now

starting to look like JavaScript :p

lol

anyways I got the idea basically how to deal with this

cool

I will work on this for now

And if this thing is simplified and looking good, then I will definitely try to work your idea of having two thread one for each EnumType

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

yeah exactly.

Let me work on this for now to make it simple and then I will try working on ClientData and Mappings class

appreciated your help David

you're welcome. good luck!