sigram commented on code in PR #2347:
URL: https://github.com/apache/solr/pull/2347#discussion_r1525968639
##########
solr/core/src/java/org/apache/solr/util/ThreadCpuTimer.java:
##########
@@ -87,23 +87,27 @@ public long getStartCpuTimeNs() {
*
* @return current value, or {@link #UNSUPPORTED} if not supported.
*/
- public long getCurrentCpuTimeNs() {
+ public long getElapsedTimerCpuNs() {
if (THREAD_MX_BEAN != null) {
return this.startCpuTimeNanos != UNSUPPORTED
- ? THREAD_MX_BEAN.getCurrentThreadCpuTime() - this.startCpuTimeNanos
+ ? getThreadTotalCpuNs() - this.startCpuTimeNanos
: UNSUPPORTED;
} else {
return UNSUPPORTED;
}
}
+ public long getThreadTotalCpuNs() {
+ return THREAD_MX_BEAN.getCurrentThreadCpuTime();
Review Comment:
Since it's a public method we need to check for null here, as in other
methods.
Also, to be consistent with other names it should be named something like
`getTotalCpuTimeNs()` ... although at this point the names become confusing
again. Maybe we should use `elapsed` and `absolute` instead?
##########
solr/core/src/java/org/apache/solr/util/ThreadCpuTimer.java:
##########
@@ -87,23 +87,27 @@ public long getStartCpuTimeNs() {
*
* @return current value, or {@link #UNSUPPORTED} if not supported.
*/
- public long getCurrentCpuTimeNs() {
+ public long getElapsedTimerCpuNs() {
Review Comment:
We're getting the elapsed CPU time, not a timer ... so a better name would
be `getElapsedCpuTimeNs()`
##########
solr/core/src/java/org/apache/solr/search/CpuAllowedLimit.java:
##########
@@ -34,7 +34,9 @@
public class CpuAllowedLimit implements QueryTimeout {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private final long limitAtNs;
+ private final long
+ timeOutAtThreadTotalCpuNs; // this is a value based on total cpu since
the inception of the
+ // thread
Review Comment:
Weird wrapping?
##########
solr/core/src/java/org/apache/solr/util/ThreadCpuTimer.java:
##########
@@ -74,7 +74,7 @@ public static boolean isSupported() {
/**
* Return the initial value of CPU time for this thread when this instance
was first created.
* NOTE: absolute value returned by this method has no meaning by itself, it
should only be used
- * when comparing elapsed time between this value and {@link
#getCurrentCpuTimeNs()}.
+ * when comparing elapsed time between this value and {@link
#getElapsedTimerCpuNs()}.
Review Comment:
This comment is not true - elapsed time is already relative to the start of
the timer, whereas the startCpuTime is an absolute value. The javadoc should
say that the comparison should be between two absolute values, ie. startCpuTime
and threadTotalCpu.
##########
solr/core/src/java/org/apache/solr/search/CpuAllowedLimit.java:
##########
@@ -57,17 +59,17 @@ public CpuAllowedLimit(SolrQueryRequest req) {
throw new IllegalArgumentException(
"Check for limit with hasCpuLimit(req) before creating a
CpuAllowedLimit");
}
- // calculate when the time limit is reached, account for the time already
spent
- limitAtNs =
- threadCpuTimer.getStartCpuTimeNs()
+ // calculate when the time when the limit is reached, e.g. account for the
time already spent
Review Comment:
"calculate the time"
--
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]
