dsmiley commented on code in PR #1243:
URL: https://github.com/apache/solr/pull/1243#discussion_r1058608729
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -99,37 +85,34 @@ protected void close() {
waitForLoadingCoresToFinish(30 * 1000);
Collection<SolrCore> coreList = new ArrayList<>();
- // Release transient core cache.
- synchronized (modifyLock) {
- if (transientSolrCoreCacheFactory != null) {
- getTransientCacheHandler().close();
- }
- }
-
// It might be possible for one of the cores to move from one list to
another while we're
// closing them. So loop through the lists until they're all empty. In
particular, the core
// could have moved from the transient list to the pendingCloses list.
- do {
- coreList.clear();
+ while (true) {
+
synchronized (modifyLock) {
- // make a copy of the cores then clear the map so the core isn't
handed out to a request
- // again
- coreList.addAll(cores.values());
- cores.clear();
- if (transientSolrCoreCacheFactory != null) {
- coreList.addAll(getTransientCacheHandler().prepareForShutdown());
+ // remove all loaded cores; add to our working list.
+ for (String name : getLoadedCoreNames()) {
+ final var core = remove(name);
+ if (core != null) {
Review Comment:
I suppose core can't be null; I just added the check to be sure. I'll
change it to an assertion.
What complexity is in SolrCores.close() that isn't needed? I suspect maybe
you mean the total work that is done (i.e. adding code that these methods
call), but I don't think that should _count_ in terms of complexity of this
method -- being able to understand what it does.
If your point wasn't actually complexity but was about performance, I think
it's a trivial matter. Consider a SolrCores is closed once :-) and I disagree
it'd be noticeably slower in any benchmark.
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -99,37 +85,34 @@ protected void close() {
waitForLoadingCoresToFinish(30 * 1000);
Collection<SolrCore> coreList = new ArrayList<>();
- // Release transient core cache.
- synchronized (modifyLock) {
- if (transientSolrCoreCacheFactory != null) {
- getTransientCacheHandler().close();
- }
- }
-
// It might be possible for one of the cores to move from one list to
another while we're
// closing them. So loop through the lists until they're all empty. In
particular, the core
// could have moved from the transient list to the pendingCloses list.
- do {
- coreList.clear();
+ while (true) {
+
synchronized (modifyLock) {
- // make a copy of the cores then clear the map so the core isn't
handed out to a request
- // again
- coreList.addAll(cores.values());
- cores.clear();
- if (transientSolrCoreCacheFactory != null) {
- coreList.addAll(getTransientCacheHandler().prepareForShutdown());
+ // remove all loaded cores; add to our working list.
+ for (String name : getLoadedCoreNames()) {
+ final var core = remove(name);
+ if (core != null) {
+ coreList.add(core);
+ }
}
coreList.addAll(pendingCloses);
pendingCloses.clear();
}
+ if (coreList.isEmpty()) {
+ break;
+ }
+
ExecutorService coreCloseExecutor =
ExecutorUtil.newMDCAwareFixedThreadPool(
Integer.MAX_VALUE, new
SolrNamedThreadFactory("coreCloseExecutor"));
try {
for (SolrCore core : coreList) {
- coreCloseExecutor.submit(
+ coreCloseExecutor.execute(
() -> {
MDCLoggingContext.setCore(core);
try {
Review Comment:
Can you comment on the error log statement you refer to? Note that the use
of MDC will have this contextual information in the log automatically.
##########
solr/core/src/java/org/apache/solr/core/SolrCores.java:
##########
@@ -99,37 +85,34 @@ protected void close() {
waitForLoadingCoresToFinish(30 * 1000);
Collection<SolrCore> coreList = new ArrayList<>();
Review Comment:
I'd be happy to do that; I debated that but your suggestion now motivates me
:-)
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
