2015-10-15 5:28 GMT+09:00 <fschumac...@apache.org>: > Author: fschumacher > Date: Wed Oct 14 20:28:55 2015 > New Revision: 1708687 > > URL: http://svn.apache.org/viewvc?rev=1708687&view=rev > Log: > Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=58489 > > Comparator should follow the rules. If first object has lastInvocation of > zero, > we should compare it to the second objects lastInvocation and vice versa. > When we do that, we can use Long#compare just as well. > > Added: > > tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestSlowQueryComparator.java > Modified: > > tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java > > Modified: > tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java?rev=1708687&r1=1708686&r2=1708687&view=diff > > ============================================================================== > --- > tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java > (original) > +++ > tomcat/trunk/modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java > Wed Oct 14 20:28:55 2015 > @@ -475,17 +475,7 @@ public class SlowQueryReport extends Abs > > @Override > public int compare(QueryStats stats1, QueryStats stats2) { > - if (stats1.lastInvocation == 0) return 1; > - if (stats2.lastInvocation == 0) return -1; > - > - long result = stats1.lastInvocation - stats2.lastInvocation; > - if (result > 0) { > - return 1; > - } else if (result == 0) { > - return 0; > - } else { > - return -1; > - } > + return Long.compare(stats1.lastInvocation, > stats2.lastInvocation); > } > } > >
Hi. I think this fix does not handle 0 value of lastInvocation correctly. The older code handles 0 value of lastInvocation as the latest QueryStats. This new code handles 0 value of lastInvocation as the oldest QueryStats. I think the fix of this bug is described in comment1. Or set current time to the lastInvocation in the constructor of QueryStats. > > Added: > tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestSlowQueryComparator.java > URL: > http://svn.apache.org/viewvc/tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestSlowQueryComparator.java?rev=1708687&view=auto > > ============================================================================== > --- > tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestSlowQueryComparator.java > (added) > +++ > tomcat/trunk/modules/jdbc-pool/src/test/java/org/apache/tomcat/jdbc/test/TestSlowQueryComparator.java > Wed Oct 14 20:28:55 2015 > @@ -0,0 +1,121 @@ > +package org.apache.tomcat.jdbc.test; > + > +import java.lang.reflect.Constructor; > +import java.lang.reflect.InvocationTargetException; > +import java.util.ArrayList; > +import java.util.Collections; > +import java.util.Comparator; > +import java.util.List; > + > +import org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport.QueryStats; > +import org.junit.Assert; > +import org.junit.Test; > + > +public class TestSlowQueryComparator { > + > + @Test > + public void testBug58489() throws ClassNotFoundException, > + InstantiationException, IllegalAccessException, > + InvocationTargetException { > + > + long[] testData = { 0, 0, 0, 1444225382010l, 0, 1444225382011l, 0, > + 1444225382012l, 0, 1444225382056l, 0, 1444225382014l, 0, > + 1444225382015l, 0, 1444225382016l, 0, 0, 1444225382017l, > 0, > + 1444225678350l, 0, 1444225680397l, 0, 1444225382018l, > + 1444225382019l, 1444225382020l, 0, 1444225382021l, 0, > + 1444225382022l, 1444225382023l > + > + }; > + > + List<QueryStats> stats = new ArrayList<>(); > + > + for (int i = 0; i < testData.length; i++) { > + QueryStats qs = new QueryStats(String.valueOf(i)); > + qs.add(0, testData[i]); > + stats.add(qs); > + } > + > + try { > + Collections.sort(stats, createComparator()); > + } catch (IllegalArgumentException e) { > + Assert.fail(e.getMessage()); > + } > + } > + > + @Test > + public void testEqualQueryStatsWithNoLastInvocation() > + throws ClassNotFoundException, InstantiationException, > + IllegalAccessException, IllegalArgumentException, > + InvocationTargetException { > + Comparator<QueryStats> queryStatsComparator = createComparator(); > + QueryStats q1 = new QueryStats("abc"); > + Assert.assertEquals(0, queryStatsComparator.compare(q1, q1)); > + } > + > + @Test > + public void testEqualQueryStatsWithLastInvocation() > + throws ClassNotFoundException, InstantiationException, > + IllegalAccessException, IllegalArgumentException, > + InvocationTargetException { > + Comparator<QueryStats> queryStatsComparator = createComparator(); > + QueryStats q1 = new QueryStats("abc"); > + q1.add(0, 100); > + Assert.assertEquals(0, queryStatsComparator.compare(q1, q1)); > + } > + > + @Test > + public void testQueryStatsOneWithLastInvocation() > + throws ClassNotFoundException, InstantiationException, > + IllegalAccessException, IllegalArgumentException, > + InvocationTargetException { > + Comparator<QueryStats> queryStatsComparator = createComparator(); > + QueryStats q1 = new QueryStats("abc"); > + QueryStats q2 = new QueryStats("def"); > + q2.add(0, 100); > + Assert.assertEquals(-1, queryStatsComparator.compare(q1, q2)); > + Assert.assertEquals(1, queryStatsComparator.compare(q2, q1)); > + } > + > + @Test > + public void testQueryStatsBothWithSameLastInvocation() > + throws ClassNotFoundException, InstantiationException, > + IllegalAccessException, IllegalArgumentException, > + InvocationTargetException { > + Comparator<QueryStats> queryStatsComparator = createComparator(); > + QueryStats q1 = new QueryStats("abc"); > + QueryStats q2 = new QueryStats("def"); > + q1.add(0, 100); > + q2.add(0, 100); > + Assert.assertEquals(0, queryStatsComparator.compare(q1, q2)); > + Assert.assertEquals(0, queryStatsComparator.compare(q2, q1)); > + } > + > + @Test > + public void testQueryStatsBothWithSomeLastInvocation() > + throws ClassNotFoundException, InstantiationException, > + IllegalAccessException, IllegalArgumentException, > + InvocationTargetException { > + Comparator<QueryStats> queryStatsComparator = createComparator(); > + QueryStats q1 = new QueryStats("abc"); > + QueryStats q2 = new QueryStats("abc"); > + q1.add(0, 100); > + q2.add(0, 150); > + Assert.assertEquals(-1, queryStatsComparator.compare(q1, q2)); > + Assert.assertEquals(1, queryStatsComparator.compare(q2, q1)); > + } > + > + private Comparator<QueryStats> createComparator() > + throws ClassNotFoundException, InstantiationException, > + IllegalAccessException, InvocationTargetException { > + Class<?> comparatorClass = Class > + > .forName("org.apache.tomcat.jdbc.pool.interceptor.SlowQueryReport$QueryStatsComparator"); > + Constructor<?> comparatorConstructor = comparatorClass > + .getDeclaredConstructors()[1]; > + comparatorConstructor.setAccessible(true); > + @SuppressWarnings("unchecked") > + Comparator<QueryStats> queryStatsComparator = > (Comparator<QueryStats>) comparatorConstructor > + .newInstance(); > + return queryStatsComparator; > + } > + > +} > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > -- > Keiichi.Fujino > <dev-h...@tomcat.apache.org> <dev-h...@tomcat.apache.org>
