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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
