FSchumacher commented on code in PR #597:
URL: https://github.com/apache/tomcat/pull/597#discussion_r1133086091
##########
modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java:
##########
@@ -222,19 +222,55 @@ protected QueryStats getQueryStats(String sql) {
return qs;
}
+ static class MiniQueryStats {
+ public final QueryStats queryStats;
+ public final long lastInvocation;
+
+ public MiniQueryStats(QueryStats queryStats) {
+ this.queryStats = queryStats;
+ this.lastInvocation = queryStats.lastInvocation;
+ }
+ }
+
+ static class MiniQueryStatsComparator implements Comparator<MiniQueryStats>
+ {
+ @Override
+ public int compare(MiniQueryStats stats1, MiniQueryStats stats2) {
+ return Long.compare(handleZero(stats1.lastInvocation),
+ handleZero(stats2.lastInvocation));
+ }
+
+ private static long handleZero(long value) {
+ return value == 0 ? Long.MAX_VALUE : value;
+ }
+ }
+
+ private MiniQueryStatsComparator miniQueryStatsComparator = new
MiniQueryStatsComparator();
+
/**
* Sort QueryStats by last invocation time
* @param queries The queries map
*/
protected void removeOldest(ConcurrentHashMap<String,QueryStats> queries) {
- ArrayList<QueryStats> list = new ArrayList<>(queries.values());
- Collections.sort(list, queryStatsComparator);
+ // Make a defensive deep-copy of the query stats list to prevent
+ // concurrent changes to the lastModified member during list-sort
+ ArrayList<MiniQueryStats> list = new ArrayList<>(queries.size());
+ for(QueryStats stats : queries.values()) {
+ list.add(new MiniQueryStats(stats));
+ }
+
+ Collections.sort(list, miniQueryStatsComparator);
int removeIndex = 0;
while (queries.size() > maxQueries) {
- String sql = list.get(removeIndex).getQuery();
- queries.remove(sql);
- if (log.isDebugEnabled()) {
- log.debug("Removing slow query, capacity reached:"+sql);
+ MiniQueryStats mqs = list.get(removeIndex);
+ // Check to see if the lastInvocation has been updated since we
+ // took our snapshot. If the timestamps disagree, it means
+ // that this item is no longer the oldest (and it likely now
+ // one of the newest).
+ if(mqs.lastInvocation == mqs.queryStats.lastInvocation) {
Review Comment:
What happens, when all (or enough) of the stats have been changed in
between? Wouldn't we get a IndexOutOfBoundException? May be, we should add a
guard to the while expression and log a message, that we could net free enough
stats.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
