views:

69

answers:

4

Hi, I have a the following use case where I'm receiving a message via JMS regarding an entity, via a unique property of it (not PK) and it requires me to update the state of the entity:

HibernateUtil.beginSession();  
HibernateUtil.beginTransaction();  
try{  
  Entity entity = dao.getEntityByUniqueProperty(propertyValue);  
  if (entity==null){  
    entity = dao.addEntityByUniqueProperty(propertyValue)  
  }  
  entity.setSomeProperty(otherPropertyValue);
  HibernateUtil.commitTransaction();
} catch (ConstraintViolationException e){  
  HibernateUtil.rollbackTransaction();  
  //Do other things additionally  
} catch (StaleStateObjectException e){  
  HibernateUtil.rollbackTransaction();  
  //Do other things additionally  
} finally {  
  HibernateUtil.closeSession();  
}

In this use case I have to be prepared for the fact that the entity which I'm trying to update is not yet created and so I request that such entity be created (a template of it to be precise with the unique property) and then I change it. My dillema is as follows: On the one hand I have two clearly different blocks and I should use the different catch clauses where appropriate BUT seeing as the end case where the entity is not there when I query but is there a ms later when I try to create it (hence ConstraintViolationException) is something that shouldn't happen too often and to insert because of that an additional commit/beginTransaction at the middle just seems waistfull.

I'm mainly concerned about the additional performance hit of the session synchronization and the JDBC connection which are done when the commit/begin occur.
Am I wrong? Am I looking for optimization where I shouldn't? Am I missing something?
Thanks in advance

+2  A: 

First of all, you need a transaction. Without it, the code above won't work because this can happen:

  • Thread 1 created unique instance
  • Thread 2 gets unique instance
  • Thread 2 sets other property
  • Thread 1 sets other property
  • Thread 1 flushes caches
  • Thread 2 flushes caches

Question: Will the DB be consistent in this case or not? Will it be (not) consistent in similar cases?

Always use transactions. DBs are optimized for it. Start to think about your design if you have a problem. Like when you have to process thousands of messages per second and your performance tools show that this code has become a bottleneck. Don't trust your gut feeling here.

Aaron Digulla
Unless I misunderstood the OP is asking how expensive a txn is because he is considering the case where ConstraintViolationException is thrown and he has to handle doing the corner case with a separate/new transaction. Rather than whether to use txns at all.
Mike Q
@MikeQ: You're right but that's not the issue. He might get performance that's a bit better but he'll corrupt his data at the same time.
Aaron Digulla
+2  A: 

A hibernate transaction is pretty much just a wrapper around a DB transaction. So it is expensive as the DB transaction would be.

As always with optimisation usually it is better to have clear and safe code over trying to eek out that extra 1% performance. BUT I don't know your use case. If the above is getting called a few times a second then don't worry about the performance. If it is getting called a couple of hundred times a second then it may be a problem.

If you have a performance issue then measure/time/profile the code until you find the problem. Often you can make an assumption that the problem is in one place when it is actually somewhere else.

In your case above I would do the following

  • Put a while loop round your code (all of it, incl the session open/close)
  • If it hits the ConstraintViolationException block log and continue so rather than coding up some extra logic just have it try again, it will then find the new row that another transaction added and update appropriately.
  • It it works ok then break out of the loop

EDIT: How I would do this....

// loop as it is possible we get a retryable exception, see below
while (true){
    HibernateUtil.beginSession();
    HibernateUtil.beginTransaction();  
    try{  
      Entity entity = dao.getEntityByUniqueProperty(propertyValue);  
      if (entity==null){  
        entity = dao.addEntityByUniqueProperty(propertyValue)  
      }  
      entity.setSomeProperty(otherPropertyValue);
      HibernateUtil.commitTransaction();

      // success
      break;

    } catch (ConstraintViolationException e){  
      HibernateUtil.rollbackTransaction();

      // this is ok, another txn must have added the entity at the same time
      // try again and it will find the entity this time
      continue;

    } catch (StaleStateObjectException e){  
      HibernateUtil.rollbackTransaction();  
      //Do other things additionally  

    } finally {  
      HibernateUtil.closeSession();  
    }
}
Mike Q
+1 for: "...Often you can make an assumption that the problem is in one place when it is actually somewhere else." that's true.Regarding the fact that "A hibernate transaction is pretty much just a wrapper around a DB transaction. So it is expensive as the DB transaction would be." how expensive is a DB transaction? roughly? Thanks
Ittai
You can probably do a couple of hundred small DB transactions per second. Ballpark.
Mike Q
A: 

I think I've actually found the specific question to my use case and that is to open the transaction only when actually needed and so I save the premature performance dillema:
HibernateUtil.beginSession();
Entity entity = dao.getEntityByUniqueProperty(propertyValue);
if (entity==null){
HibernateUtil.beginTransaction();
try{
entity = dao.addEntityByUniqueProperty(propertyValue)
HibernateUtil.commitTransaction();
} catch (ConstraintViolationException e){
HibernateUtil.rollbackTransaction();
HibernateUtil.closeSession();
HibernateUtil.beginSession();
entity = dao.getEntityByUniqueProperty(propertyValue);
//Do other things additionally
}
}
entity.setSomeProperty(otherPropertyValue); HibernateUtil.commitTransaction(); } catch (StaleStateObjectException e){
HibernateUtil.rollbackTransaction();
//Do other things additionally
} finally {
HibernateUtil.closeSession();
}

This will enable me to localize the specific risks in each Transaction and also avoid commiting and opening a transaction when it is unneeded. Thanks for your comments.

Ittai
I wouldn't advise this, best practice is to always have a transaction even for selects, see Aaron's answer. Plus you'll find that the JDBC driver is probably issuing a transaction calls anyway. Honestly unless you're trying to do hundreds a second it is unlikely to be a problem. I'll edit my answer with how I would solve this problem.
Mike Q
@Mike This is not true, not using transactions for read-only queries is perfectly fine.
Pascal Thivent
A: 

No matter what you'll do, write operations can't be done outside a transaction, Hibernate will complain if there is no ongoing transaction and throw an exception. So no choice here.

Now, your use case - a findOrCreate() - is not a trivial one. As you mention, you can face a race condition:

T1: BEGIN TX;

T2: BEGIN TX;

T1: getEntityByUniqueProperty("foo"); //returns null

T1: getEntityByUniqueProperty("foo"); //returns null

T1: addEntityByUniqueProperty("foo");
T1: COMMIT; //row inserted

T1: addEntityByUniqueProperty("foo");
T2: COMMIT; //constraint violation

So you'll have to either

  1. synchronize the code (which is only an option IF you are running in a single JVM) ~or~
  2. lock the whole table (ouch!) ~or~
  3. deal with it and implement some kind of retry mechanism.

Personally, I'd go for option 3. Performance wise, it's the best choice an is actually a common pattern, especially when working with messaging and high concurrency.

Pascal Thivent