Author: jleroux
Date: Fri Jul 8 22:58:35 2011 New Revision: 1144537 URL: http://svn.apache.org/viewvc?rev=1144537&view=rev Log: Closes "SequenceUtil may generate duplicate IDs in Load Balancing mode" https://issues.apache.org/jira/browse/OFBIZ-2353 Pb: If OFBiz is deployed on 2 servers in Load Balancing Mode, SequenceUtil will generate duplicate IDs because synchronization is done at JVM level instead of doing it in DB. The OFBIZ-2353 SELECT FOR UPDATE solution.patch provides a simple mean to quickly resolve this issue. I did not remove the bank loop (useless since there is a DB contention). I created a cluster general property. But since, most of the time, when you use a cluster you use also a sole database with a delegator having distributed-cache-clear-enabled set to true, I retrieve this information to automatically use SELECT FOR UPDATE in this case This has been tested on a cluster with JMeter. BTW I found that UserLoginHistory can throw errors because it does not use a SeqId in PK but only login+fromDate. I guess there are other cases like that but in reality it's unlikely that someone log on 2 diff machines with the same login at the same time (ms), just a JMeter side effect... Modified: ofbiz/trunk/framework/common/config/general.properties ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java Modified: ofbiz/trunk/framework/common/config/general.properties URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/config/general.properties?rev=1144537&r1=1144536&r2=1144537&view=diff ============================================================================== --- ofbiz/trunk/framework/common/config/general.properties (original) +++ ofbiz/trunk/framework/common/config/general.properties Fri Jul 8 22:58:35 2011 @@ -130,3 +130,6 @@ http.localhost=ABQIAAAAtt0d8djaYFkk8N5LJ # -- Y if you want to display the multi-tenant textbox in the login page multitenant=N + +# -- Y if you use a cluster +cluster=N Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java?rev=1144537&r1=1144536&r2=1144537&view=diff ============================================================================== --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java (original) +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/Delegator.java Fri Jul 8 22:58:35 2011 @@ -1222,4 +1222,10 @@ public interface Delegator { */ public boolean getEnabledJMS(); + /** + * Get use of Distributed Cache Clear mechanism status + * @return boolean true if this delegator uses a Distributed Cache Clear mechanism + */ + public boolean useDistributedCacheClear(); + } Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java?rev=1144537&r1=1144536&r2=1144537&view=diff ============================================================================== --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java (original) +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/GenericDelegator.java Fri Jul 8 22:58:35 2011 @@ -2468,7 +2468,7 @@ public class GenericDelegator implements synchronized (this) { if (sequencer == null) { ModelEntity seqEntity = this.getModelEntity("SequenceValueItem"); - sequencer = new SequenceUtil(this.getEntityHelperInfo("SequenceValueItem"), seqEntity, "seqName", "seqId"); + sequencer = new SequenceUtil(this, this.getEntityHelperInfo("SequenceValueItem"), seqEntity, "seqName", "seqId"); } } } @@ -2918,4 +2918,12 @@ public class GenericDelegator implements public boolean getEnabledJMS() { return this.enableJMS; } + + /* (non-Javadoc) + * @see org.ofbiz.entity.Delegator#getEnableJMS() + */ + public boolean useDistributedCacheClear() { + return this.getDelegatorInfo().useDistributedCacheClear; + } + } Modified: ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java?rev=1144537&r1=1144536&r2=1144537&view=diff ============================================================================== --- ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java (original) +++ ofbiz/trunk/framework/entity/src/org/ofbiz/entity/util/SequenceUtil.java Fri Jul 8 22:58:35 2011 @@ -28,6 +28,8 @@ import java.util.Map; import javax.transaction.Transaction; import org.ofbiz.base.util.Debug; +import org.ofbiz.base.util.UtilProperties; +import org.ofbiz.entity.GenericDelegator; import org.ofbiz.entity.GenericEntityException; import org.ofbiz.entity.datasource.GenericHelperInfo; import org.ofbiz.entity.jdbc.ConnectionFactory; @@ -50,8 +52,9 @@ public class SequenceUtil { private final String tableName; private final String nameColName; private final String idColName; + private final boolean clustered; - public SequenceUtil(GenericHelperInfo helperInfo, ModelEntity seqEntity, String nameFieldName, String idFieldName) { + public SequenceUtil(GenericDelegator delegator, GenericHelperInfo helperInfo, ModelEntity seqEntity, String nameFieldName, String idFieldName) { this.helperInfo = helperInfo; if (seqEntity == null) { throw new IllegalArgumentException("The sequence model entity was null but is required."); @@ -76,6 +79,7 @@ public class SequenceUtil { bankSize = seqEntity.getSequenceBankSize().longValue(); } this.bankSize = bankSize; + clustered = delegator.useDistributedCacheClear() || "Y".equals(UtilProperties.getPropertyValue("general.properties", "clustered")); } public Long getNextSeqId(String seqName, long staggerMax, ModelEntity seqModelEntity) { @@ -182,8 +186,8 @@ public class SequenceUtil { this.seqName + "; start of loop val1=" + val1 + ", val2=" + val2 + ", bankSize=" + bankSize, module); // not sure if this synchronized block is totally necessary, the method is synchronized but it does do a wait/sleep - //outside of this block, and this is the really sensitive block, so making sure it is isolated; there is some overhead - //to this, but very bad things can happen if we try to do too many of these at once for a single sequencer + // outside of this block, and this is the really sensitive block, so making sure it is isolated; there is some overhead + // to this, but very bad things can happen if we try to do too many of these at once for a single sequencer synchronized (this) { Transaction suspendedTransaction = null; try { @@ -218,8 +222,11 @@ public class SequenceUtil { // we shouldn't need this, and some TX managers complain about it, so not including it: connection.setAutoCommit(false); stmt = connection.createStatement(); - - sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'"; + if (clustered) { + sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'" + " FOR UPDATE"; + } else { + sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'"; + } rs = stmt.executeQuery(sql); boolean gotVal1 = false; if (rs.next()) { @@ -241,8 +248,12 @@ public class SequenceUtil { if (stmt.executeUpdate(sql) <= 0) { throw new GenericEntityException("[SequenceUtil.SequenceBank.fillBank] update failed, no rows changes for seqName: " + seqName); } + if (clustered) { + sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'" + " FOR UPDATE"; - sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'"; + } else { + sql = "SELECT " + SequenceUtil.this.idColName + " FROM " + SequenceUtil.this.tableName + " WHERE " + SequenceUtil.this.nameColName + "='" + this.seqName + "'"; + } rs = stmt.executeQuery(sql); boolean gotVal2 = false; if (rs.next()) { |
Free forum by Nabble | Edit this page |