5
\$\begingroup\$

I am trying to implement lock by which I want to avoid reads from happening whenever I am doing a write on my three maps.

Requirements:

  • Reads block until all three maps have been set for the first time.
  • Now second time, If I am updating the maps, I can still return all the three old maps value(before the updates are done on all three maps) or it should block and return me all the new three maps value whenever the updates are done on all the three maps.

As I have three Maps - primary , secondary and tertiary so it should return either all the new values of three updated maps or it should return all the old values of the map. Basically, while updating I don't want to return primary having old values, secondary having having new values, and tertiary with new values.

It should be consistent, either it should return old values or it should return new values after/while updating the maps. In my case, updating of maps will happen once in three months or four months.

And from the main application thread, reads will be happening on the getter of those three maps at a rate of 1000 requests per second. So the get call on the three maps are really performance critical. I was thinking to use ReentrantLock or ReentrantReadWriteLock but got stick to CountDownLatch because of simplicity-

Below is my ClientData class in which I am using CountDownLatch -

public class ClientData {

    public static class Mappings {
        public final Map<String, Map<Integer, String>> primary;
        public final Map<String, Map<Integer, String>> secondary;
        public final Map<String, Map<Integer, String>> tertiary;

        public Mappings(
            Map<String, Map<Integer, String>> primary,
            Map<String, Map<Integer, String>> secondary,
            Map<String, Map<Integer, String>> tertiary
        ) {
            this.primary = primary;
            this.secondary = secondary;
            this.tertiary = tertiary;
        }
    }

    private static final AtomicReference<Mappings> mappings = new AtomicReference<>();
    private static final CountDownLatch hasBeenInitialized = new CountDownLatch(1);

    public static Mappings getMappings() {
        try {
            hasBeenInitialized.await();
            return mappings.get();
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            throw new IllegalStateException(e);
        }
    }

    public static void setMappings(
        Map<String, Map<Integer, String>> primary,
        Map<String, Map<Integer, String>> secondary,
        Map<String, Map<Integer, String>> tertiary
    ) {
        setMappings(new Mappings(primary, secondary, tertiary));
    }

    public static void setMappings(Mappings newMappings) {
        mappings.set(newMappings);
        hasBeenInitialized.countDown();
    }
}

And below is my background thread code. It will get the data from my service URL and keep on running every 10 minutes once my application has started up. It will then parse the data coming from the URL and store it in a ClientData class variable in those three maps as shown above.

public class TempScheduler {

    private final ScheduledExecutorService scheduler = Executors.newScheduledThreadPool(1);

        public void startScheduler() {
            final ScheduledFuture<?> taskHandle = scheduler.scheduleAtFixedRate(new Runnable() {
                public void run() {
                try {
                    callServers();
                } catch (Exception ex) {
                    ex.printStackTrace();
                }
                }
            }, 0, 10, TimeUnit.MINUTES);
        }
    }

    // call the servers and get the data and then parse 
    // the response.
    private void callServers() {
        String url = "url";
        RestTemplate restTemplate = new RestTemplate();
        String response = restTemplate.getForObject(url, String.class);
        parseResponse(response);

    }

    // parse the response and store it in a variable
    private void parseResponse(String response) {
        //...       
        ConcurrentHashMap<String, Map<Integer, String>> primaryTables = null;
        ConcurrentHashMap<String, Map<Integer, String>> secondaryTables = null;
        ConcurrentHashMap<String, Map<Integer, String>> tertiaryTables = null;

        //...

        // store the data in ClientData class variables if anything has changed  
        // which can be used by other threads
        if(changed) {
            ClientData.setMappings(primaryTables, secondaryTables, tertiaryTables);
        }
    }
}

Problem Statement:

Is there any way I can improve the above ClientData class somehow? Any kind of performance improvements? Or by using any other ways instead of CountDownLatch?

I will be using ClientData class in main application thread like this -

Mappings mappings = ClientData.getMappings(); 
// use mappings.primary 
// use mappings.secondary 
// use mappings.tertiary

I am suspecting, I can avoid AtomicReference here -

The atomic reference may not actually be required since both get and set operations synchronize on hasBeenInitialized.

\$\endgroup\$
  • \$\begingroup\$ I had assumed in your previous question that the maps couldn't be modified once set. If that's the case, there's no need for ConcurrentHashMap here since they are written by a single thread and safely published before being read. \$\endgroup\$ – David Harkness Apr 29 '14 at 22:00
  • \$\begingroup\$ @DavidHarkness: You talking about inside TempScheduler class? Right? \$\endgroup\$ – arsenal Apr 29 '14 at 22:14
  • \$\begingroup\$ Correct. If those maps created in a single background thread, there's no need for a concurrent map. \$\endgroup\$ – David Harkness Apr 29 '14 at 22:18
  • \$\begingroup\$ Yeah. good point. I will replace that with LinkedHashMap then. \$\endgroup\$ – arsenal Apr 29 '14 at 22:24
  • \$\begingroup\$ If you don't need insertion-order traversal you can use HashMap instead of LinkedHashMap to save some memory. ConcurrentHashMap didn't provide this feature, so I doubt you're depending on it. \$\endgroup\$ – David Harkness Apr 29 '14 at 22:32
3
\$\begingroup\$

Going through some smaller items in your code. Your Mappings class is public, which is fine, but it should also be final. There is no reason for people to extend your class. The constructor should be private, and all the Map fields should be private as well, but you should have getters for those maps... getPrimary(), getSecondary(), etc.

On the ClientData class, the method setMappings(Mappings...) content should be merged in to the setMappings(Map ..., Map.. Map) method, and that method should not be public either, assuming the background thread is defined in the same package as you.

Restricting these permissions will make your application better defined.

The remaining concern with your code is whether the Map instances themselves are Mutable (well, they are mutable, but should they be?). I would recommend making them readonly using Collections.unmodifiableMap() which will prevent the actual Map content from becoming corrupted by some thread polluting the data.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.