[
https://issues.apache.org/jira/browse/SOLR-9658?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16926186#comment-16926186
]
Hoss Man commented on SOLR-9658:
--------------------------------
{quote}refactored cache impls to allow inserting synthetic entries, and changed
the unit tests to use these methods. It turned out that the management of
oldestEntry needs to be improved in all caches when we allow the creation time
in more recently added entries to go back...
{quote}
Ah interesting ... IIUC the existing code (in ConcurrentLFUCache for example)
just tracks "lastAccessed" for each cache entry (and "oldest" for the cache as
a whole) via an incremented counter across all entries – but now you're using
actual NANO_SECOND timestamps. This seems like an "ok" change (the API has
never exposed these "lastAccessed" values correct?) but I just want to double
check since you've looked at this & thought about it more then me: do you see
any risk here? (ie: please don't let me talk you into an Impl change that's "a
bad idea" just because it makes the kind of test I was advocating easier to
write)
Feedback on other aspects of the patch (all minor and/or nitpicks – in
generally this all seems solid) ...
* AFAICT there should no longer be any need to modify TimeSource /
TestTimeSource since tests no longer use/need advanceMs, correct ?
* {{SolrCache.MAX_IDLE_TIME}} doesn't seem to have a name consistent w/the
other variables in that interface ... seems like it should be
{{SolrCache.MAX_IDLE_TIME_PARAM}} ?
** There are also a couple of places in LFUCache and LRUCache (where other
existing {{*_PARAM}} constants are used) that seem to use the string literal
{{"maxIdleTime"}} instead of using that new variable.
* IIUC This isn't a mistake, it's a deliberate "clean up" change because the
existing code includes this {{put(RAM_BYTES_USED_PARAM, ...)}} twice a few
lines apart, correct? ...
{code:java}
- map.put(RAM_BYTES_USED_PARAM, ramBytesUsed());
+ map.put("cumulative_idleEvictions", cidleEvictions);
{code}
* Is there any reason not to make these final in both ConcurrentLFUCache &
ConcurrentLRUCache?
{code:java}
private TimeSource timeSource = TimeSource.NANO_TIME;
private AtomicLong oldestEntry = new AtomicLong(0L);
{code}
* re: this line in {{TestLFUCache.testMaxIdleTimeEviction}} ...
** {{assertEquals("markAndSweep spurious run", 1, sweepFinished.getCount());}}
** a more thread safe way to have this type of assertion...
{code:java}
final AtomicLong numSweepsStarted = new AtomicLong(0); // NEW
final CountDownLatch sweepFinished = new CountDownLatch(1);
ConcurrentLRUCache<String, Accountable> cache = new ConcurrentLRUCache<>(6, 5,
5, 6, false, false, null, IDLE_TIME_SEC) {
@Override
public void markAndSweep() {
numSweepsStarted.incrementAndGet(); // NEW
super.markAndSweep();
sweepFinished.countDown();
}
};
...
assertEquals("markAndSweep spurious runs", 0L, numSweepsStarted.get()); //
CHANGED
{code}
** I think that pattern exists in another test as well?
* we need to make sure the javadocs & ref-guide are updated to cover this new
option, and be clear to users on how it interacts with other things (ie: that
the idle sweep happens before the other sweeps and trumps things like the
"entry size" checks)
> Caches should have an optional way to clean if idle for 'x' mins
> ----------------------------------------------------------------
>
> Key: SOLR-9658
> URL: https://issues.apache.org/jira/browse/SOLR-9658
> Project: Solr
> Issue Type: New Feature
> Reporter: Noble Paul
> Assignee: Andrzej Bialecki
> Priority: Major
> Fix For: 8.3
>
> Attachments: SOLR-9658.patch, SOLR-9658.patch, SOLR-9658.patch,
> SOLR-9658.patch, SOLR-9658.patch, SOLR-9658.patch
>
>
> If a cache is idle for long, it consumes precious memory. It should be
> configurable to clear the cache if it was not accessed for 'x' secs. The
> cache configuration can have an extra config {{maxIdleTime}} . if we wish it
> to the cleaned after 10 mins of inactivity set it to {{maxIdleTime=600}}.
> [~dragonsinth] would it be a solution for the memory leak you mentioned?
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
