3
\$\begingroup\$

In one of my object I needed a sequence I would use to increment an Id field just like we often do with classical databases. As I have not found a suitable solution, I wrote one. This solution uses objectify.

The first approach I came with was to have a class with a static AtomicInteger field like this:

public class MySequence {
  static AtomicInteger sequence = new AtomicInteger(0);
}

I would then just call it each time I need it.

Problem:

Every time the server is restarted, the sequence is too. So we need to persist the last value.

Second approach:

Persist the last value with objectify:

 public class MySequence {
        private static AtomicInteger sequence = new AtomicInteger(0);

        private static List<PersistSequence> l = null;


private static PersistSequence newPs = null;
    /**
     * @param user_id the id of the user we want to get the last value of the sequence
     * @return last value of PersistSequence
     */
    public static int getNextValue( String user_id){
        if( sequence.get() == 0){//first time we run it or it has been restarted
            l = ofy().load().type( PersistSequence.class).filter("user_id", user_id).limit(1).list();//grabs PersistSequence object bounded to user_id
            if( l != null){
                int dataStoreSeq = -1;
                if ( l.size() > 0) dataStoreSeq = l.get(0).getLastId();
                if ( dataStoreSeq > sequence.get()) sequence.set( dataStoreSeq);
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }else{
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }   
        }else{
            persistCounter( user_id, sequence.incrementAndGet());
            return sequence.get();
        }
    }

    /**
     * @param user_id the id of the user
     * @param lastId last used Id
     */
    private static void persistCounter( String user_id, int lastId){
        //we only want one PersistSequence object per user so we delete the previous one before saving the new PersistSequence object
        List<Key<PersistSequence>> keys = ofy().load().type(PersistSequence.class).filter("user_id", user_id).keys().list();
        ofy().delete().keys(keys).now();
        //save the new one
        newPs = new PersistSequence();
        newPs.setLastId(lastId);
        newPs.setUserId(user_id);
        ofy().save().entities( newPs).now();
        } 
    }

I am happy with this code as it works well. However, I wonder if it still will rock if there is a high volume of users calling it.

\$\endgroup\$
0

1 Answer 1

1
\$\begingroup\$

Shouldn't get id twice

I'm concerned with this code:

        persistCounter( user_id, sequence.incrementAndGet());
        return sequence.get();

After you atomically increment and get the counter, you call sequence.get() again. That seems wrong because what happens if some other thread incremented the counter in the meantime? I think your code should be:

        int newId = sequence.incrementAndGet();
        persistCounter(user_id, newId);
        return newId;

Repeated code

Also, I noticed that the same block of code (the one above) appears three times in a row:

    public static int getNextValue( String user_id){
        if( sequence.get() == 0){//first time we run it or it has been restarted
            l = ofy().load().type(PersistSequence.class).
                    filter("user_id", user_id).limit(1).list();
            if( l != null){
                int dataStoreSeq = -1;
                if ( l.size() > 0) dataStoreSeq = l.get(0).getLastId();
                if ( dataStoreSeq > sequence.get()) sequence.set( dataStoreSeq);
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }else{
                persistCounter( user_id, sequence.incrementAndGet());
                return sequence.get();
            }   
        }else{
            persistCounter( user_id, sequence.incrementAndGet());
            return sequence.get();
        }
    }

Since that code happens for every case, you can simplify your function by extracting that common code and placing it right before the function returns:

    public static int getNextValue( String user_id){
        if( sequence.get() == 0){//first time we run it or it has been restarted
            l = ofy().load().type(PersistSequence.class).
                    filter("user_id", user_id).limit(1).list();
            if( l != null){
                int dataStoreSeq = -1;
                if ( l.size() > 0) dataStoreSeq = l.get(0).getLastId();
                if ( dataStoreSeq > sequence.get()) sequence.set( dataStoreSeq);
            }
        }
        int newId = sequence.incrementAndGet();
        persistCounter(user_id, newId);
        return newId;
    }

Concurrency issues in persistCounter()

It's possible that persistCounter() will get called with out of order lastId arguments. For example, it's possible that you might get called with lastId of 5 and then later get called with a lastId of 4. So I think before you save the new counter, you should compare it with the old one to make sure that are actually saving a newer counter.

Also, there may be a concurrency problem with this function. What happens if one thread tries to save 4 and another tries to save 5 at the same time? Could it possibly end up saving 4 instead of 5?

\$\endgroup\$
2
  • \$\begingroup\$ You right with the code getNextValue method. I will correct it accordingly. \$\endgroup\$ Commented Aug 1, 2015 at 19:14
  • \$\begingroup\$ Also persisCounter() is private and it should only be called when the value of lastId has been retrieved. Please, see the code above. For concurrency issue I have added a call to ReentrantLock. \$\endgroup\$ Commented Aug 1, 2015 at 19:20

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.