You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/10/05 12:43:47 UTC
[GitHub] [solr] dsmiley commented on a change in pull request #326: SOLR-15301 Eliminate repetitive index size calculation for Solr metrics
dsmiley commented on a change in pull request #326:
URL: https://github.com/apache/solr/pull/326#discussion_r722196433
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -491,22 +490,27 @@ public long getIndexSize() {
}
public long getCachedIndexSize() {
- if (indexSize.get() == null) {
- if (log.isDebugEnabled()) {
- log.debug("Recalculating index size for {}", getName());
+ SolrRequestInfo requestInfo = SolrRequestInfo.getRequestInfo();
+ if (requestInfo != null) {
+ Map<Object,Object> contextMap = requestInfo.getReq().getContext();
+ Long cachedIndexSize = (Long)contextMap.get(cachedIndexSizeKeyName(getName()));
Review comment:
You could use the computeIfAbsent pattern which is more idiomatic than this older Java style? Granted you'd probably lose the log for re-using a previous index size but I don't think the log is important.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -491,22 +490,27 @@ public long getIndexSize() {
}
public long getCachedIndexSize() {
Review comment:
I think this method should be named simply getIndexSize(), and then the method that is currently named that could be computeIndexSize (plus a javadoc to ask the caller to call getIndexSize? WDYT? Thus the cached aspect is an implementation detail.
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCore.java
##########
@@ -491,22 +490,27 @@ public long getIndexSize() {
}
public long getCachedIndexSize() {
- if (indexSize.get() == null) {
- if (log.isDebugEnabled()) {
- log.debug("Recalculating index size for {}", getName());
+ SolrRequestInfo requestInfo = SolrRequestInfo.getRequestInfo();
+ if (requestInfo != null) {
+ Map<Object,Object> contextMap = requestInfo.getReq().getContext();
+ Long cachedIndexSize = (Long)contextMap.get(cachedIndexSizeKeyName(getName()));
+ if (cachedIndexSize == null) {
+ if (log.isDebugEnabled()) {
+ log.debug("Recalculating index size for {}", getName());
+ }
+ cachedIndexSize = getIndexSize();
+ contextMap.put(cachedIndexSizeKeyName(getName()), cachedIndexSize);
+ } else if (log.isDebugEnabled()) {
+ log.debug("reusing previous index size for {}", getName());
}
- indexSize.set(getIndexSize());
- } else if (log.isDebugEnabled()) {
- log.debug("reusing previous index size for {}", getName());
+ return cachedIndexSize;
+ } else {
+ return getIndexSize();
}
- return indexSize.get();
}
- public void clearIndexSizeCache() {
- if (log.isDebugEnabled()) {
- log.debug("Clearing index size cache for {}", getName());
- }
- indexSize.remove();
+ private String cachedIndexSizeKeyName(String name) {
Review comment:
I was thinking maybe we'd need another map inside the context to key by core name but your approach here is simpler and adequate. Please add a comment on why we're doing this -- it may not be apparent that we're avoiding collisions across cores for the same metrics request.
--
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-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org