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.

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

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

Conversation

ChristopherSchultz
Copy link
Contributor

Copy link
Member

@aooohan aooohan left a comment

Choose a reason for hiding this comment

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

Another unrelated thing, it might be better to unify the coding style?
eg:

for() {
   // code
}

not:

for()
 // code

not

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants