[
https://issues.apache.org/jira/browse/SOLR-9658?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16902561#comment-16902561
]
Hoss Man commented on SOLR-9658:
--------------------------------
* i don't see anything that updates {{oldestEntryNs}} except
{{markAndSweepByIdleTime}} ?
** this means that {{markAndSweep()}} may unneccessarily call
{{markAndSweepByIdleTime()}} (looping over every entry) even if everything
older then the maxIdleTime has already been purged by earlier method calls like
{{markAndSweepByCacheSize()}} or {{markAndSweepByRamSize()}}
** off the top of my head, i can't think of an efficient way to "update"
{{oldestEntryNs}} in some place like {{postRemoveEntry()}} w/o scanning every
cache entry again, but...
** why not move {{markAndSweepByIdleTime()}} _before_
{{markAndSweepByCacheSize()}} and {{markAndSweepByRamSize()}} ?
*** since the {{postRemoveEntry()}} calls made as a result of any eviction due
to idle time *can* (and already do) efficiently update the results of
{{size()}} and {{ramBytesUsed()}} that could potentially save the need for
those additional scans of the cache in many situations.
* rather then complicating the patch by changing the constructor of the
{{CleanupThread}} class(es) to take in the maxIdle values directly, why not
read that info from a (new) method on the ConcurrentXXXCache objects already
passed to the constructors?
** with some small tweaks to the while loop, the {{wait()}} call could actual
read this value dynamically from the cache element, eliminating the need to
call {{setRunCleanupThread()}} from inside {{setMaxIdleTime()}} in the event
that the value is changed dynamically.
*** which is currently broken anyway since {{setRunCleanupThread()}} is
currently a No-Op if {{this.runCleanupThread}} is true and {{cleanupThread}} is
already non-null.
** assuming {{CleanupThread}} is changed to dynamically read the maxIdleTime
directly from the cache, {{setMaxIdleTime()}} could just call {{wakeThread()}}
if the new maxIdleTime is less then the old maxIdleTime
*** or leave the call to {{setRunCleanupThread()}} as is, but change the {{if
(cleanupThread == null)}} condition of {{setRunCleanupThread()}} to have an
"else" code path that calls {{wakeThread()}} so it will call {{markAndSweep()}}
(with the udpated settings) and then re-wait (with the new maxIdleTime)
* although not likely to be problematic in practice, you've broken backcompat
on the public "ConcurrentXXXCache" class(es) by adding an arg to the
constructor.
** i would suggest adding a new constructor instead, and making the old one
call the new one with "-1" – if for no other reason then to simplify the touch
points / discussion in the patch...
** ie: in order to make this change, you had to modify both
{{TestJavaBinCodec}} and {{TemplateUpdateProcessorFactory}} – but you wound up
not using a backcompat equivilent value in {{TemplateUpdateProcessorFactory}}
so your changes actually modify the behavior of that (end user facing class) in
an undocumented way (that users can't override, and may actually have some
noticible performance impacts on "put" since that existing usage doens't
involve the cleanup thread) which should be discussed before committing (but
are largely unrelated to the goals in this jira)
* under no circumstances should we be committing new test code that makes
arbitrary {{Thread.sleep(5000)}} calls
** i am willing to say categorically that this approach: DOES. NOT. WORK. –
and has represented an overwelming percentage of the root causes of our tests
being unreliable
*** there is no garuntee the JVM will sleep as long as you ask it too
(particularly on virtual hardware)
*** there is no garuntee that "background threads/logic" will be
scheduled/finished during the "sleep"
** it is far better to add whatever {{@lucene.internal}} methods we need to
"hook into" the core code from test code and have white-box / grey-box tests
that ensure methods get called when we expect, ex:
*** if we want to test that the user level configuration results in the
appropriate values being set on the underlying objects, we should add public
getter methods for those values to those classes, and have the test reach into
the SolrCore to get those objects and assert the expected results on those
methods (NOT just "wait" to see the code run and have the expected side effects)
*** if we want to test that {{ConcurrentXXXCache.markAndSweep()}} gets called
by the {{CleanupThread}} _eventually_ when maxIdle time is configured even if
nothing calls {{wakeThread()}} then we should use a mock/subclass of the
ConcurrentXXXCache that overrides {{markAndSweep()}} to set a latch that we can
{{await(...)}} on from the test code.
*** if we want to test that calls to {{ConcurrentXXXCache.markAndSweep()}}
result in items being removed if their {{createTime}} is "too old" then we
should add a special internal only version of "put" that let's us add
synthetically created {{CacheEntry}} instances with synthetically set
{{createTime}} values and assert that they are removed after calling
{{markAndSweep()}}
*** etc...
> 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
>
>
> 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
(v7.6.14#76016)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
