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. Feel free to change if you want. > And why check "query" and then parse "queryTimeout"? > Good catch > > This check does not appear in the original patch. > > > + timeout = Integer.parseInt(queryTimeout); > > + } > > + } catch (NumberFormatException nfe) { > > + timeout = 0; > > + } > > + return timeout; > > + } > > + > > + /** > > + * @return the queryTimeout > > + */ > > + public String getQueryTimeout() { > > + return queryTimeout ; > > + } > > + > > + /** > > + * @param resultVariable the variable name in which results will be > stored > > Javadoc is wrong. > Fixed > > > + */ > > + public void setQueryTimeout(String queryTimeout) { > > + this.queryTimeout = queryTimeout; > > } > > > > public String getQuery() { > > > > Modified: > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java > > URL: > http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java?rev=1491213&r1=1491212&r2=1491213&view=diff > > > ============================================================================== > > --- > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java > (original) > > +++ > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/JDBCTestElementBeanInfoSupport.java > Sun Jun 9 13:07:32 2013 > > @@ -42,28 +42,33 @@ public abstract class JDBCTestElementBea > > "queryArgumentsTypes", // $NON-NLS-1$ > > "variableNames", // $NON-NLS-1$ > > "resultVariable", // $NON-NLS-1$ > > + "queryTimeout" // $NON-NLS-1$ > > }); > > > > PropertyDescriptor p = property("dataSource"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > - p.setValue(DEFAULT, ""); > > + p.setValue(DEFAULT, ""); // $NON-NLS-1$ > > > > p = property("queryArguments"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > - p.setValue(DEFAULT, ""); > > + p.setValue(DEFAULT, ""); // $NON-NLS-1$ > > > > p = property("queryArgumentsTypes"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > - p.setValue(DEFAULT, ""); > > + p.setValue(DEFAULT, ""); // $NON-NLS-1$ > > > > p = property("variableNames"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > - p.setValue(DEFAULT, ""); > > + p.setValue(DEFAULT, ""); // $NON-NLS-1$ > > > > p = property("resultVariable"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > - p.setValue(DEFAULT, ""); > > + p.setValue(DEFAULT, ""); // $NON-NLS-1$ > > > > + p = property("queryTimeout"); // $NON-NLS-1$ > > + p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > + p.setValue(DEFAULT, ""); > > + > > p = property("queryType"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > p.setValue(DEFAULT, AbstractJDBCTestElement.SELECT); > > @@ -82,7 +87,7 @@ public abstract class JDBCTestElementBea > > > > p = property("query"); // $NON-NLS-1$ > > p.setValue(NOT_UNDEFINED, Boolean.TRUE); > > - p.setValue(DEFAULT, ""); > > + p.setValue(DEFAULT, ""); // $NON-NLS-1$ > > p.setPropertyEditorClass(TextAreaEditor.class); > > > > } > > > > Modified: > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties > > URL: > http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties?rev=1491213&r1=1491212&r2=1491213&view=diff > > > ============================================================================== > > --- > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties > (original) > > +++ > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources.properties > Sun Jun 9 13:07:32 2013 > > @@ -30,4 +30,5 @@ variableNames.displayName=Variable names > > variableNames.shortDescription=Output variable names for each column > (comma separated) > > resultVariable.displayName=Result variable name > > resultVariable.shortDescription=Name of the JMeter variable that stores > the result set objects in a list of maps for looking up results by column > name. > > - > > +queryTimeout.displayName=Query timeout > > +queryTimeout.shortDescription=The timeout of statement measured in > seconds > > \ No newline at end of file > > > > Modified: > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties > > URL: > http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties?rev=1491213&r1=1491212&r2=1491213&view=diff > > > ============================================================================== > > --- > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties > (original) > > +++ > jmeter/trunk/src/protocol/jdbc/org/apache/jmeter/protocol/jdbc/sampler/JDBCSamplerResources_fr.properties > Sun Jun 9 13:07:32 2013 > > @@ -31,3 +31,5 @@ sql.displayName=Requ\u00EAte SQL > > varName.displayName=Nom de liaison avec le pool > > variableNames.displayName=Noms des variables > > variableNames.shortDescription=Noms des variables en sortie pour chaque > colonne (s\u00E9par\u00E9s par des virgules) > > +queryTimeout.displayName=Timeout de la requ\u00EAte > > +queryTimeout.shortDescription=Timeout de le requ\u00EAte en secondes > > \ No newline at end of file > > > > Modified: jmeter/trunk/xdocs/changes.xml > > URL: > http://svn.apache.org/viewvc/jmeter/trunk/xdocs/changes.xml?rev=1491213&r1=1491212&r2=1491213&view=diff > > > ============================================================================== > > --- jmeter/trunk/xdocs/changes.xml (original) > > +++ jmeter/trunk/xdocs/changes.xml Sun Jun 9 13:07:32 2013 > > @@ -174,6 +174,7 @@ Transaction Controller now sets Response > > <li><bugzilla>54798</bugzilla> - Using subject from EML-file for SMTP > Sampler</li> > > <li><bugzilla>54759</bugzilla> - SSLPeerUnverifiedException using HTTPS > , property documented</li> > > <li><bugzilla>54896</bugzilla> - JUnit sampler gives only “failed to > create an instance of the class†message with constructor problems</li> > > +<li><bugzilla>55084</bugzilla> - Add timeout support for JDBC > Request</li> > > </ul> > > > > <h3>Controllers</h3> > > > > > -- Cordialement. Philippe Mouawad.
