On 9 June 2013 14:40, Philippe Mouawad <[email protected]> wrote: > Hello, > Fixed thanks for review. > > Regards > Philippe > > On Sun, Jun 9, 2013 at 3:30 PM, sebb <[email protected]> wrote: > >> On 9 June 2013 14:07, <[email protected]> wrote: >> > Author: pmouawad >> > Date: Sun Jun 9 13:07:32 2013 >> > New Revision: 1491213 >> > >> > URL: http://svn.apache.org/r1491213 >> > Log: >> > Bug 55084 - Add timeout support for JDBC Request >> > Bugzilla Id: 55084 >> >> There are some problems with this patch as detailed below. >> >> Please fix ASAP. >> >> > Modified: >> > >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java >> > >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java >> > >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties >> > >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties >> > jmeter/trunk/xdocs/changes.xml >> > >> > Modified: >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java >> > URL: >> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java?rev=1491213&r1=1491212&r2=1491213&view=diff >> > >> ============================================================================== >> > --- >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java >> (original) >> > +++ >> jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/AbstractJDBCTestElement.java >> Sun Jun 9 13:07:32 2013 >> > @@ -35,6 +35,7 @@ import java.util.List; >> > import java.util.Map; >> > import java.util.concurrent.ConcurrentHashMap; >> > >> > +import org.apache.commons.lang3.StringUtils; >> > import org.apache.jmeter.samplers.SampleResult; >> > import org.apache.jmeter.save.CSVSaveService; >> > import org.apache.jmeter.testelement.AbstractTestElement; >> > @@ -111,7 +112,8 @@ public abstract class AbstractJDBCTestEl >> > private String queryArguments = ""; // $NON-NLS-1$ >> > private String queryArgumentsTypes = ""; // $NON-NLS-1$ >> > private String variableNames = ""; // $NON-NLS-1$ >> > - private String resultVariable = ""; >> > + private String resultVariable = ""; // $NON-NLS-1$ >> > + private String queryTimeout = ""; // $NON-NLS-1$ >> > >> > /** >> > * Cache of PreparedStatements stored in a per-connection basis. >> Each entry of this >> > @@ -142,6 +144,7 @@ public abstract class AbstractJDBCTestEl >> > String _queryType = getQueryType(); >> > if (SELECT.equals(_queryType)) { >> > stmt = conn.createStatement(); >> > + stmt.setQueryTimeout(getIntegerQueryTimeout()); >> > ResultSet rs = null; >> > try { >> > rs = stmt.executeQuery(getQuery()); >> > @@ -159,6 +162,7 @@ public abstract class AbstractJDBCTestEl >> > return sb.getBytes(ENCODING); >> > } else if (UPDATE.equals(_queryType)) { >> > stmt = conn.createStatement(); >> > + stmt.setQueryTimeout(getIntegerQueryTimeout()); >> > stmt.executeUpdate(getQuery()); >> > int updateCount = stmt.getUpdateCount(); >> > String results = updateCount + " updates"; >> > @@ -332,9 +336,15 @@ public abstract class AbstractJDBCTestEl >> > } else { >> > pstmt = conn.prepareStatement(getQuery()); >> > } >> > + pstmt.setQueryTimeout(getIntegerQueryTimeout()); >> > // PreparedStatementMap is associated to one connection so >> > // 2 threads cannot use the same PreparedStatement map at >> the same time >> > preparedStatementMap.put(getQuery(), pstmt); >> > + } else { >> > + int timeoutInS = getIntegerQueryTimeout(); >> > + if(pstmt.getQueryTimeout() != timeoutInS) { >> > + pstmt.setQueryTimeout(getIntegerQueryTimeout()); >> > + } >> > } >> > pstmt.clearParameters(); >> > return pstmt; >> > @@ -457,6 +467,35 @@ public abstract class AbstractJDBCTestEl >> > } catch (SQLException e) { >> > log.warn("Error closing ResultSet", e); >> > } >> > + } >> > + >> > + /** >> > + * @return the integer representation queryTimeout >> > + */ >> > + public int getIntegerQueryTimeout() { >> > + int timeout = 0; >> > + try { >> > + if(StringUtils.isNumeric(query)) { >> >> Not sure what the point of the numeric check is - why not just let the >> parseInt fail? >> > To avoid Exception throwing which has higher cost than testing for numeric.
The saving if any is likely to be marginal. > Feel free to change if you want. Since a non-numeric value is not valid, why should it matter if it takes slightly longer to process? I think the validation is unnecessary here.
