ChristopherSchultz commented on code in PR #597:
URL: https://github.com/apache/tomcat/pull/597#discussion_r1133119845
##########
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:
Good question. If 100% of the stats have been updated, then we will remove
nothing, no space will be gained, and we will end up running off the end of the
list, throwing an error.
I see a few options:
1. YOLO! Just let the loop run and hope for the best! j/k
2. Ensure `removeIndex` doesn't grow too high, then just stop
3. Do (2), and repeat the sort+loop until we remove enough entries to meet
cache-size expectations
I think probably (2) is sufficient. Thoughts?
--
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
