aooohan commented on code in PR #597:
URL: https://github.com/apache/tomcat/pull/597#discussion_r1282608560
##########
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:
Maybe use `PriorityQueue` instead of `ArrayList` is a better way? The
latter will not have the problem of `removeIndex` being too high. See below:
```java
protected void removeOldest(ConcurrentHashMap<String,QueryStats> queries) {
int removeCount = queries.size() - maxQueries;
if (removeCount <= 0) {
return;
}
// Make a defensive deep-copy of the query stats list to prevent
// concurrent changes to the lastModified member during list-sort
PriorityQueue<MiniQueryStats> queue = new
PriorityQueue<>(queries.size(), miniQueryStatsComparator);
for(QueryStats stats : queries.values()) {
queue.offer(new MiniQueryStats(stats));
}
while (removeCount > 0 && !queue.isEmpty()) {
MiniQueryStats mqs = queue.poll();
// 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) {
String sql = mqs.queryStats.query;
queries.remove(sql);
removeCount--;
if (log.isDebugEnabled()) log.debug("Removing slow query,
capacity reached:"+sql);
}
}
}
```
--
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
