dsmiley commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1700915813
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -47,7 +46,6 @@ public class CollectionPropertiesZkStateReader implements
Closeable {
private volatile boolean closed = false;
private final SolrZkClient zkClient;
- private final ZkStateReader zkStateReader;
Review Comment:
glad to see this gone!
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -217,7 +222,8 @@ public void process(WatchedEvent event) {
*/
void refreshAndWatch(boolean notifyWatchers) {
try {
- synchronized (watchedCollectionProps) { // making decisions based on
the result of a get...
Review Comment:
the comment is still reasonable; could be it's own line once the lock is
obtained
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -217,7 +222,8 @@ public void process(WatchedEvent event) {
*/
void refreshAndWatch(boolean notifyWatchers) {
try {
- synchronized (watchedCollectionProps) { // making decisions based on
the result of a get...
+ Object collectionLock = getCollectionLock(coll);
+ synchronized (collectionLock) { // Lock on individual collection
Review Comment:
as one line then no need to declare & name "collectionLock". Also the
comment doesn't add anything that is already extremely obvious
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -402,10 +412,11 @@ public void removeCollectionPropsWatcher(String
collection, CollectionPropsWatch
v.stateWatchers.remove(watcher);
if (v.canBeRemoved()) {
// don't want this to happen in middle of other blocks that might
add it back.
- synchronized (watchedCollectionProps) {
+ Object collectionLock = getCollectionLock(collection);
+ synchronized (collectionLock) {
Review Comment:
combine as one line
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -104,46 +103,52 @@ public CollectionPropertiesZkStateReader(ZkStateReader
zkStateReader) {
* @return a map representing the key/value properties for the collection.
*/
public Map<String, String> getCollectionProperties(final String collection,
long cacheForMillis) {
- synchronized (watchedCollectionProps) { // synchronized on the specific
collection
- Watcher watcher = null;
- if (cacheForMillis > 0) {
- watcher =
- collectionPropsWatchers.compute(
- collection,
- (c, w) ->
- w == null ? new PropsWatcher(c, cacheForMillis) :
w.renew(cacheForMillis));
- }
- VersionedCollectionProps vprops = watchedCollectionProps.get(collection);
- boolean haveUnexpiredProps = vprops != null && vprops.cacheUntilNs >
System.nanoTime();
- long untilNs =
- System.nanoTime() + TimeUnit.NANOSECONDS.convert(cacheForMillis,
TimeUnit.MILLISECONDS);
- Map<String, String> properties;
+ Watcher watcher = null; // synchronized on the specific collection
+ if (cacheForMillis > 0) {
+ watcher =
+ collectionPropsWatchers.compute(
+ collection,
+ (c, w) -> w == null ? new PropsWatcher(c, cacheForMillis) :
w.renew(cacheForMillis));
+ }
+ VersionedCollectionProps vprops = watchedCollectionProps.get(collection);
+ boolean haveUnexpiredProps = vprops != null && vprops.cacheUntilNs >
System.nanoTime();
+ long untilNs =
+ System.nanoTime() + TimeUnit.NANOSECONDS.convert(cacheForMillis,
TimeUnit.MILLISECONDS);
+ if (haveUnexpiredProps) {
+ vprops.cacheUntilNs = Math.max(vprops.cacheUntilNs, untilNs);
+ return vprops.props;
+ }
+ // Synchronize only when properties are expired or not present
+ Object collectionLock = getCollectionLock(collection);
+ synchronized (collectionLock) {
Review Comment:
combine to one line
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -77,12 +75,13 @@ public class CollectionPropertiesZkStateReader implements
Closeable {
private final ExecutorService notifications =
ExecutorUtil.newMDCAwareCachedThreadPool("cachecleaner");
- // only kept to identify if the cleaner has already been started.
- private Future<?> collectionPropsCacheCleaner;
+ // identify if the cleaner has already been started.
+ private final AtomicBoolean collectionPropsCacheCleanerInitialized = new
AtomicBoolean(false);
Review Comment:
There is only one cache cleaner; can't it simply be a Thread instead of an
ExecutorService? Then we can easily see if it's running or not if needed.
--
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]
