Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use a deep copy of query stats whose values won't change during sorting. #597

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use a deep copy of query stats whose values won't change during sorting. #597

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4ca773
Use a deep copy of query stats whose values won't change during sorting.
ChristopherSchultz Mar 8, 2023
f237d05
Match coding style.
ChristopherSchultz Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 42 additions & 6 deletions modules/jdbc-pool/src/main/java/org/apache/tomcat/jdbc/pool/interceptor/SlowQueryReport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had number two in mind, with a warn message at the end, when we could not free enough stats.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use PriorityQueue instead of ArrayList is a better way? The latter will not have the problem of removeIndex being too high. See below:

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);
            }
            
        }
    }

String sql = mqs.queryStats.query;
queries.remove(sql);
if (log.isDebugEnabled()) log.debug("Removing slow query, capacity reached:"+sql);
}
removeIndex++;
}
Expand Down