6
\$\begingroup\$

This singleton is responsible for counting every pageview (called Routes in our case) we get by id, until the timer runs out and commits the results to the database in one query. Then the counter gets reset and the timer starts again.

I made this to reduce the amount of connections happening on our database at a given time.

Our pages are HttpServlet subclasses, so they get called concurrently. Our viewcounter should support this.

This is the code I think would be right for this:

@Singleton
@ConcurrencyManagement(BEAN)
public class RouteViewcounterSingleton {

    private ConcurrentHashMap<Integer, Integer> viewCounter;

    @EJB
    private RouteViewcountDAO routeViewcountDAO;

    @Inject
    private RouteDAO routeDao;

    @Resource
    TimerService timerService;

    @PostConstruct
    public void initialize() {
        viewCounter = new ConcurrentHashMap<>();
    }

    public void incrementViewCount(Integer routeId) {
        if (viewCounter.containsKey(routeId)) {
            Integer counter = viewCounter.get(routeId);
            counter++;
            viewCounter.put(routeId, counter);
        } else {
            viewCounter.put(routeId, 1);
        }
    }

    @Timeout
    public void programmaticTimeout(Timer timer) {
        Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.SEVERE, "ViewCounter timer timeout: " + DateTime.now(), "");
    }

    @Schedule(second = "0", minute = "*/10", hour = "*", info = "ViewCounterTimer")
    public void scheduleTimer() {
        if (viewCounter != null) {
            int i = 0;
            for (Map.Entry<Integer, Integer> entrySet : viewCounter.entrySet()) {
                try {
                    Integer routeId = entrySet.getKey();
                    Integer viewCount = entrySet.getValue();
                    Route route = routeDao.find(routeId);
                    RouteViewcount routeViewcount = routeViewcountDAO.find(route);
                    if (routeViewcount != null) {
                        routeViewcount.setViews(routeViewcount.getViews() + viewCount);
                        routeViewcountDAO.edit(routeViewcount);
                    } else {
                        routeViewcountDAO.create(new RouteViewcount(route, viewCount));
                    }
                    i++;
                } catch (MultipleResultsException | NoResultException | AttributeNotUniqueException | VersionConflictException ex) {
                    Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.SEVERE, null, ex);
                }
            }
            Logger.getLogger(RouteViewcounterSingleton.class.getName()).log(Level.INFO, "Viewcounter persisted for " + i + " routes on :" + DateTime.now(), "");
            viewCounter.clear();
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$

This method is broken in two ways. First, multiple threads can access the counter at the same time, and based on ordering, you may lose some updates. Second, if the counter is being created for the first time by multiple threads, you may create the counter multiple times.

public void incrementViewCount(Integer routeId) {
    if (viewCounter.containsKey(routeId)) {
        Integer counter = viewCounter.get(routeId);
        counter++;
        viewCounter.put(routeId, counter);
    } else {
        viewCounter.put(routeId, 1);
    }
}

In addition, you are not clear()ing the map in a thread-safe way. You should strongly consider an existing counter class, like Guava's AtomicLongMap. Writing a thread-safe counter is surprisingly non-trivial.

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