murblanc commented on a change in pull request #1504:
URL: https://github.com/apache/lucene-solr/pull/1504#discussion_r427930548
##########
File path:
solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/PolicyHelper.java
##########
@@ -429,87 +440,124 @@ private void release(SessionWrapper sessionWrapper) {
* The session can be used by others while the caller is performing
operations
*/
private void returnSession(SessionWrapper sessionWrapper) {
- TimeSource timeSource = sessionWrapper.session != null ?
sessionWrapper.session.cloudManager.getTimeSource() : TimeSource.NANO_TIME;
+ boolean present;
synchronized (lockObj) {
sessionWrapper.status = Status.EXECUTING;
- if (log.isDebugEnabled()) {
- log.debug("returnSession, curr-time {} sessionWrapper.createTime {},
this.sessionWrapper.createTime {} "
- , time(timeSource, MILLISECONDS),
- sessionWrapper.createTime,
- this.sessionWrapper.createTime);
- }
- if (sessionWrapper.createTime == this.sessionWrapper.createTime) {
- //this session was used for computing new operations and this can
now be used for other
- // computing
- this.sessionWrapper = sessionWrapper;
+ present = sessionWrapperSet.contains(sessionWrapper);
- //one thread who is waiting for this need to be notified.
- lockObj.notify();
- } else {
- log.debug("create time NOT SAME {} ",
SessionWrapper.DEFAULT_INSTANCE.createTime);
- //else just ignore it
- }
+ // wake up single thread waiting for a session return (ok if not woken
up, wait is short)
+ lockObj.notify();
}
+ // Logging
+ if (present) {
+ if (log.isDebugEnabled()) {
+ log.debug("returnSession {}", sessionWrapper.getCreateTime());
+ }
+ } else {
+ log.warn("returning unknown session {} ",
sessionWrapper.getCreateTime());
+ }
}
- public SessionWrapper get(SolrCloudManager cloudManager) throws
IOException, InterruptedException {
+ public SessionWrapper get(SolrCloudManager cloudManager, boolean
allowWait) throws IOException, InterruptedException {
Review comment:
Thanks. I feel it makes the flow a bit harder to read and the savings
are not huge so I prefer to stick to the original structure of this method.
(the memory impact is negligible IMO. There's also an additional call to
hasNonExpiredSession in the proposal but again no big deal)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
