Fixes bug 29708, Fixes bug 29936: Improve performance and memory problems
authorUnai Martirena <unai.martirena@openbravo.com>
Tue, 02 Jun 2015 09:12:55 +0200
changeset 26858 654d4ad1872f
parent 26857 807a09f2e66c
child 26860 0e4099b33747
Fixes bug 29708, Fixes bug 29936: Improve performance and memory problems

2 Problems are happening in Costing Background process:

1) Java Heap error: This problem happens because getTransactions method can load on memory a lot of objects and even an ScrollableResult is being used the following sentence 'OBDal.getInstance().getConnection().setHoldability(ResultSet.HOLD_CURSORS_OVER_COMMIT)' prevents in fact to scroll properly the ScrollableResult. To avoid this issue the query of getTransactions method has been changed to return only 1000 records. So, instead of calling just once this method it is done once each 1000 records.

Also instead of returning a list of 1000 MaterialTransaction objects, that can be a relatively big amount of objects to load in memory, only the Ids are returned and each id the it is instanced on each iteration.

2) Performance problems: The process has a performance issue because it does a commit on each iteration of the list. Now, this has been changed to do commit every 1000 records, and in case that an exception is raised on a certain iteration a new method has been added in catch clause to calculate the cost of the transactions that have been rolledback but which there were properly calculated.
src/org/openbravo/costing/CostingBackground.java
--- a/src/org/openbravo/costing/CostingBackground.java	Thu Jun 04 17:02:09 2015 +0200
+++ b/src/org/openbravo/costing/CostingBackground.java	Tue Jun 02 09:12:55 2015 +0200
@@ -18,8 +18,6 @@
  */
 package org.openbravo.costing;
 
-import java.sql.ResultSet;
-import java.sql.SQLException;
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
@@ -28,8 +26,6 @@
 
 import org.apache.log4j.Logger;
 import org.hibernate.Query;
-import org.hibernate.ScrollMode;
-import org.hibernate.ScrollableResults;
 import org.openbravo.base.exception.OBException;
 import org.openbravo.base.provider.OBProvider;
 import org.openbravo.dal.core.OBContext;
@@ -69,6 +65,8 @@
     logger = bundle.getLogger();
     OBError result = new OBError();
     List<String> orgsWithRule = new ArrayList<String>();
+    List<String> trxs = null;
+    int counter = 0, total = 0, batch = 0;
     try {
       OBContext.setAdminMode(false);
       result.setType("Success");
@@ -104,31 +102,29 @@
       // Fix the Not Processed flag for those Transactions with Cost Not Calculated
       setNotProcessedWhenNotCalculatedTransactions(orgsWithRule);
 
-      ScrollableResults trxs = getTransactions(orgsWithRule);
-      int counter = 0;
-      try {
-        while (trxs.next()) {
-          MaterialTransaction transaction = (MaterialTransaction) trxs.get()[0];
+      trxs = getTransactionsBatch(orgsWithRule);
+      total = trxs.size();
+      while (counter < maxTransactions) {
+        batch++;
+        for (String trxId : trxs) {
           counter++;
+          MaterialTransaction transaction = OBDal.getInstance().get(MaterialTransaction.class,
+              trxId);
           if ("S".equals(transaction.getCostingStatus())) {
             // Do not calculate trx in skip status.
             continue;
           }
           log4j.debug("Start transaction process: " + transaction.getId());
-          OBDal.getInstance().refresh(transaction);
           CostingServer transactionCost = new CostingServer(transaction);
           transactionCost.process();
-          log4j.debug("Transaction processed: " + counter + "/" + maxTransactions);
-          // If cost has been calculated successfully do a commit.
-          OBDal.getInstance().getConnection(true).commit();
-          if (counter % 1 == 0) {
-            OBDal.getInstance().getSession().clear();
-          }
+          log4j.debug("Transaction processed: " + counter + "/" + maxTransactions + " batch: "
+              + batch);
         }
-      } finally {
-        try {
-          trxs.close();
-        } catch (Exception ignore) {
+        OBDal.getInstance().getConnection(true).commit();
+        OBDal.getInstance().getSession().clear();
+        if (counter < maxTransactions) {
+          trxs = getTransactionsBatch(orgsWithRule);
+          total += trxs.size();
         }
       }
 
@@ -136,6 +132,7 @@
       bundle.setResult(result);
     } catch (OBException e) {
       OBDal.getInstance().rollbackAndClose();
+      calculateCorrectTrxWhileRollback(trxs, counter, total);
       String message = OBMessageUtils.parseTranslation(bundle.getConnection(), bundle.getContext()
           .toVars(), OBContext.getOBContext().getLanguage().getLanguage(), e.getMessage());
       result.setMessage(message);
@@ -146,6 +143,7 @@
       return;
     } catch (Exception e) {
       OBDal.getInstance().rollbackAndClose();
+      calculateCorrectTrxWhileRollback(trxs, counter, total);
       result = OBMessageUtils.translateError(bundle.getConnection(), bundle.getContext().toVars(),
           OBContext.getOBContext().getLanguage().getLanguage(), e.getMessage());
       result.setType("Error");
@@ -205,12 +203,15 @@
     OBDal.getInstance().flush();
   }
 
-  private ScrollableResults getTransactions(List<String> orgsWithRule) {
+  @SuppressWarnings("unchecked")
+  private List<String> getTransactionsBatch(List<String> orgsWithRule) {
     StringBuffer where = new StringBuffer();
-    where.append(" as trx");
+    where.append(" select trx." + MaterialTransaction.PROPERTY_ID + " as id ");
+    where.append(" from " + MaterialTransaction.ENTITY_NAME + " as trx");
     where.append(" join trx." + MaterialTransaction.PROPERTY_PRODUCT + " as p");
     where.append("\n , " + org.openbravo.model.ad.domain.List.ENTITY_NAME + " as trxtype");
     where.append("\n where trx." + MaterialTransaction.PROPERTY_ISPROCESSED + " = false");
+    where.append("   and trx." + MaterialTransaction.PROPERTY_ISCOSTCALCULATED + " = false");
     where.append("   and p." + Product.PROPERTY_PRODUCTTYPE + " = 'I'");
     where.append("   and p." + Product.PROPERTY_STOCKED + " = true");
     where.append("   and trxtype." + CostAdjustmentUtils.propADListReference + ".id = :refid");
@@ -222,25 +223,70 @@
     where.append(" , trxtype." + CostAdjustmentUtils.propADListPriority);
     where.append(" , trx." + MaterialTransaction.PROPERTY_MOVEMENTQUANTITY + " desc");
     where.append(" , trx." + MaterialTransaction.PROPERTY_ID);
-    OBQuery<MaterialTransaction> trxQry = OBDal.getInstance().createQuery(
-        MaterialTransaction.class, where.toString());
+    Query trxQry = OBDal.getInstance().getSession().createQuery(where.toString());
 
-    trxQry.setNamedParameter("refid", CostAdjustmentUtils.MovementTypeRefID);
-    trxQry.setNamedParameter("now", new Date());
-    trxQry.setFilterOnReadableOrganization(false);
-    trxQry.setNamedParameter("orgs", orgsWithRule);
+    trxQry.setParameter("refid", CostAdjustmentUtils.MovementTypeRefID);
+    trxQry.setParameter("now", new Date());
+    trxQry.setParameterList("orgs", orgsWithRule);
 
     if (maxTransactions == 0) {
-      maxTransactions = trxQry.count();
+      maxTransactions = count(orgsWithRule);
     }
+    trxQry.setMaxResults(1000);
+    return trxQry.list();
+  }
+
+  /**
+   * Return count of transactions to be calculated
+   */
+  private int count(List<String> orgsWithRule) {
+    StringBuffer where = new StringBuffer();
+    where.append(" select count(*) ");
+    where.append(" from " + MaterialTransaction.ENTITY_NAME + " as trx");
+    where.append(" join trx." + MaterialTransaction.PROPERTY_PRODUCT + " as p");
+    where.append("\n , " + org.openbravo.model.ad.domain.List.ENTITY_NAME + " as trxtype");
+    where.append("\n where trx." + MaterialTransaction.PROPERTY_ISPROCESSED + " = false");
+    where.append("   and trx." + MaterialTransaction.PROPERTY_ISCOSTCALCULATED + " = false");
+    where.append("   and p." + Product.PROPERTY_PRODUCTTYPE + " = 'I'");
+    where.append("   and p." + Product.PROPERTY_STOCKED + " = true");
+    where.append("   and trxtype." + CostAdjustmentUtils.propADListReference + ".id = :refid");
+    where.append("   and trxtype." + CostAdjustmentUtils.propADListValue + " = trx."
+        + MaterialTransaction.PROPERTY_MOVEMENTTYPE);
+    where.append("   and trx." + MaterialTransaction.PROPERTY_TRANSACTIONPROCESSDATE + " <= :now");
+    where.append("   and trx." + MaterialTransaction.PROPERTY_ORGANIZATION + ".id in (:orgs)");
+    Query trxQry = OBDal.getInstance().getSession().createQuery(where.toString());
+
+    trxQry.setParameter("refid", CostAdjustmentUtils.MovementTypeRefID);
+    trxQry.setParameter("now", new Date());
+    trxQry.setParameterList("orgs", orgsWithRule);
+
+    return ((Number) trxQry.uniqueResult()).intValue();
+  }
+
+  private void calculateCorrectTrxWhileRollback(List<String> trxs, int _counter, int total) {
+    int correctTrx = _counter - 1;
+    int counter = total - 1000;
     try {
-      OBDal.getInstance().getConnection().setHoldability(ResultSet.HOLD_CURSORS_OVER_COMMIT);
-    } catch (SQLException e) {
-      log4j.error("error: " + e.getMessage(), e);
-      throw new OBException(e.getMessage());
+      for (String trxId : trxs) {
+        if (counter == correctTrx) {
+          break;
+        }
+        MaterialTransaction transaction = OBDal.getInstance().get(MaterialTransaction.class, trxId);
+        counter++;
+        if ("S".equals(transaction.getCostingStatus())) {
+          // Do not calculate trx in skip status.
+          continue;
+        }
+        CostingServer transactionCost = new CostingServer(transaction);
+        transactionCost.process();
+      }
+      OBDal.getInstance().getConnection(true).commit();
+      OBDal.getInstance().getSession().clear();
+    } catch (OBException e1) {
+      OBDal.getInstance().rollbackAndClose();
+    } catch (Exception e1) {
+      OBDal.getInstance().rollbackAndClose();
     }
-
-    return trxQry.scroll(ScrollMode.FORWARD_ONLY);
   }
 
   private void initializeMtransCostDateAcct() throws Exception {