You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/10/08 10:27:26 UTC
[GitHub] [lucene-solr] sigram opened a new pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
sigram opened a new pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962
This is just the API part of the ticket, separated from PR-1758 to facilitate reviews of pure API.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#issuecomment-716508967
@noblepaul I added back the default implementation of `ClusterEventProducer`, and an example implementation of `CollectionsRepairEventListener` to illustrate how to re-implement the `autoAddReplicas` functionality. Please review.
(BTW, ignore the messed up merge history here... I made a mistake when merging, but the final diff with master is now correct).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r512365317
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
There is no need to have a `ClusterEventProducer` if the user does not register it.
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -252,6 +253,11 @@ public CoreLoadFailure(CoreDescriptor cd, Exception loadFailure) {
getZkController().getOverseer() != null &&
!getZkController().getOverseer().isClosed(),
(r) -> this.runAsync(r));
+
+ private final ClusterEventProducerFactory clusterEventProducerFactory = new ClusterEventProducerFactory(this);
Review comment:
Does it have to be a field? It can be a local variable as well
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517213017
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
Review comment:
This class is only used by a single thread, right?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517290048
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
Review comment:
Yes.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r512473892
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
I'm not sure. If we treat this as a part of core functionality (the ability to produce and listen to cluster-level events) then we need a rudimentary default implementation to be always present.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r513938423
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
> AFAIK there is no mechanism in plugins to require the presence of another plugin.
There is no reason why we can't implement it. I think we should. Everything should be initialized as lazily as possible.
The add command for a `ClusterEventListener` has to do an extra check to see if a `ClusterEventProducer` is registered. If not, just register it
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] muse-dev[bot] commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r516960732
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducerBase.java
##########
@@ -0,0 +1,85 @@
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ *
+ */
+public abstract class ClusterEventProducerBase implements ClusterEventProducer {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ protected final Map<ClusterEvent.EventType, Set<ClusterEventListener>> listeners = new ConcurrentHashMap<>();
+ protected volatile State state = State.STOPPED;
+ protected final CoreContainer cc;
+
+ protected ClusterEventProducerBase(CoreContainer cc) {
+ this.cc = cc;
+ }
+
+ @Override
+ public void registerListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+ if (eventTypes == null || eventTypes.length == 0) {
+ eventTypes = ClusterEvent.EventType.values();
+ }
+ for (ClusterEvent.EventType type : eventTypes) {
+ if (!getSupportedEventTypes().contains(type)) {
+ log.warn("event type {} not supported yet.", type);
+ continue;
+ }
+ // to avoid removing no-longer empty set in unregister
+ synchronized (listeners) {
+ listeners.computeIfAbsent(type, t -> ConcurrentHashMap.newKeySet())
+ .add(listener);
+ }
+ }
+ }
+
+ @Override
+ public void unregisterListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+ if (eventTypes == null || eventTypes.length == 0) {
+ eventTypes = ClusterEvent.EventType.values();
+ }
+ synchronized (listeners) {
+ for (ClusterEvent.EventType type : eventTypes) {
+ Set<ClusterEventListener> perType = listeners.get(type);
+ if (perType != null) {
+ perType.remove(listener);
+ if (perType.isEmpty()) {
+ listeners.remove(type);
+ }
+ }
+ }
+ }
+ }
+
+ @Override
+ public State getState() {
+ return state;
+ }
+
+ public Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners() {
+ return listeners;
+ }
+
+ public CoreContainer getCoreContainer() {
+ return cc;
+ }
+
+ public abstract Set<ClusterEvent.EventType> getSupportedEventTypes();
+
+ protected void fireEvent(ClusterEvent event) {
+ listeners.getOrDefault(event.getType(), Collections.emptySet())
Review comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `ClusterEventProducerBase.fireEvent(...)` reads without synchronization from container `this.listeners` via call to `Map.getOrDefault(...)`. Potentially races with write in method `ClusterEventProducerBase.unregisterListener(...)`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517207532
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private ContainerPluginsRegistry.PluginRegistryListener initialPluginListener;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ super(cc);
+ initialPluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
Review comment:
Wouldn't code be easier to read/maintain if named classes were defined rather than anonymous ones? They would have names that help understand their function, class javadoc etc... I feel the anonymous classes in this PR have enough content to justify a non anonymous implementation. That's likely just a personal preference of mine so feel free to ignore.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] muse-dev[bot] commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517470119
##########
File path: solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java
##########
@@ -100,6 +101,15 @@ public void writeMap(EntryWriter ew) throws IOException {
currentPlugins.forEach(ew.getBiConsumer());
}
+ @Override
+ public void close() throws IOException {
+ currentPlugins.values().forEach(apiInfo -> {
Review comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `ContainerPluginsRegistry.close()` reads without synchronization from container `this.currentPlugins` via call to `Map.values()`. Potentially races with write in method `ContainerPluginsRegistry.refresh()`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
##########
File path: solr/core/src/java/org/apache/solr/api/ContainerPluginsRegistry.java
##########
@@ -100,6 +101,15 @@ public void writeMap(EntryWriter ew) throws IOException {
currentPlugins.forEach(ew.getBiConsumer());
Review comment:
*THREAD_SAFETY_VIOLATION:* Read/Write race. Non-private method `ContainerPluginsRegistry.writeMap(...)` reads without synchronization from container `this.currentPlugins` via call to `Map.forEach(...)`. Potentially races with write in method `ContainerPluginsRegistry.refresh()`.
Reporting because another access to the same memory occurs on a background thread, although this access may not.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517293581
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private ContainerPluginsRegistry.PluginRegistryListener initialPluginListener;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ super(cc);
+ initialPluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
+ @Override
+ public void added(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(ContainerPluginsRegistry.ApiInfo old, ContainerPluginsRegistry.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ @Override
+ public Set<ClusterEvent.EventType> getSupportedEventTypes() {
+ return NoOpProducer.ALL_EVENT_TYPES;
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public ContainerPluginsRegistry.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.ContainerPluginsRegistry.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public DelegatingClusterEventProducer create(ContainerPluginsRegistry plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final DelegatingClusterEventProducer clusterEventProducer = new DelegatingClusterEventProducer(cc);
+ // since this is a ClusterSingleton, register it as such
+ cc.getClusterSingletons().getSingletons().put(ClusterEventProducer.PLUGIN_NAME +"_delegate", clusterEventProducer);
+ ContainerPluginsRegistry.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer.setDelegate((ClusterEventProducer) clusterEventProducerInfo.getInstance());
+ } else {
+ // use the default NoOp impl
+ }
+ // transfer those listeners that were already registered to the initial impl
+ transferListeners(clusterEventProducer, plugins);
Review comment:
No. At this point in the initialization of CoreContainer any source of events will be ignored anyway (because the factory is basically a no-op implementation that doesn't generate any events), and listeners are being transferred before the final implementation is installed.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517287374
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducerBase.java
##########
@@ -0,0 +1,85 @@
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ *
+ */
+public abstract class ClusterEventProducerBase implements ClusterEventProducer {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ protected final Map<ClusterEvent.EventType, Set<ClusterEventListener>> listeners = new ConcurrentHashMap<>();
+ protected volatile State state = State.STOPPED;
+ protected final CoreContainer cc;
+
+ protected ClusterEventProducerBase(CoreContainer cc) {
+ this.cc = cc;
+ }
+
+ @Override
+ public void registerListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+ if (eventTypes == null || eventTypes.length == 0) {
+ eventTypes = ClusterEvent.EventType.values();
+ }
+ for (ClusterEvent.EventType type : eventTypes) {
+ if (!getSupportedEventTypes().contains(type)) {
+ log.warn("event type {} not supported yet.", type);
+ continue;
+ }
+ // to avoid removing no-longer empty set in unregister
+ synchronized (listeners) {
+ listeners.computeIfAbsent(type, t -> ConcurrentHashMap.newKeySet())
+ .add(listener);
+ }
+ }
+ }
+
+ @Override
+ public void unregisterListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+ if (eventTypes == null || eventTypes.length == 0) {
+ eventTypes = ClusterEvent.EventType.values();
+ }
+ synchronized (listeners) {
+ for (ClusterEvent.EventType type : eventTypes) {
+ Set<ClusterEventListener> perType = listeners.get(type);
+ if (perType != null) {
+ perType.remove(listener);
+ if (perType.isEmpty()) {
+ listeners.remove(type);
+ }
+ }
+ }
+ }
+ }
+
+ @Override
+ public State getState() {
+ return state;
+ }
+
+ public Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners() {
+ return listeners;
+ }
+
+ public CoreContainer getCoreContainer() {
+ return cc;
+ }
+
+ public abstract Set<ClusterEvent.EventType> getSupportedEventTypes();
+
+ protected void fireEvent(ClusterEvent event) {
+ listeners.getOrDefault(event.getType(), Collections.emptySet())
Review comment:
Good point, muse-dev :) Fixing.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r512566935
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
This is a core , opt-in functionality
core = part of core
opt-in =. no traces if user does not explicitly add an event producer
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r512473129
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -252,6 +253,11 @@ public CoreLoadFailure(CoreDescriptor cd, Exception loadFailure) {
getZkController().getOverseer() != null &&
!getZkController().getOverseer().isClosed(),
(r) -> this.runAsync(r));
+
+ private final ClusterEventProducerFactory clusterEventProducerFactory = new ClusterEventProducerFactory(this);
Review comment:
+1.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram closed pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram closed pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517218843
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private ContainerPluginsRegistry.PluginRegistryListener initialPluginListener;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ super(cc);
+ initialPluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
+ @Override
+ public void added(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(ContainerPluginsRegistry.ApiInfo old, ContainerPluginsRegistry.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ @Override
+ public Set<ClusterEvent.EventType> getSupportedEventTypes() {
+ return NoOpProducer.ALL_EVENT_TYPES;
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public ContainerPluginsRegistry.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.ContainerPluginsRegistry.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public DelegatingClusterEventProducer create(ContainerPluginsRegistry plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final DelegatingClusterEventProducer clusterEventProducer = new DelegatingClusterEventProducer(cc);
+ // since this is a ClusterSingleton, register it as such
+ cc.getClusterSingletons().getSingletons().put(ClusterEventProducer.PLUGIN_NAME +"_delegate", clusterEventProducer);
+ ContainerPluginsRegistry.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer.setDelegate((ClusterEventProducer) clusterEventProducerInfo.getInstance());
+ } else {
+ // use the default NoOp impl
+ }
+ // transfer those listeners that were already registered to the initial impl
+ transferListeners(clusterEventProducer, plugins);
+ // install plugin registry listener
+ ContainerPluginsRegistry.PluginRegistryListener pluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
+ @Override
+ public void added(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ ClusterEventListener listener = (ClusterEventListener) instance;
+ clusterEventProducer.registerListener(listener);
+ } else if (instance instanceof ClusterEventProducer) {
+ // replace the existing impl
+ if (cc.getClusterEventProducer() instanceof DelegatingClusterEventProducer) {
+ ((DelegatingClusterEventProducer) cc.getClusterEventProducer())
+ .setDelegate((ClusterEventProducer) instance);
+ } else {
+ log.warn("Can't configure plugin-based ClusterEventProducer while CoreContainer is still loading - " +
+ " using existing implementation {}", cc.getClusterEventProducer().getClass().getName());
+ }
+ }
+ }
+
+ @Override
+ public void deleted(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ ClusterEventListener listener = (ClusterEventListener) instance;
+ clusterEventProducer.unregisterListener(listener);
+ } else if (instance instanceof ClusterEventProducer) {
+ // replace the existing impl with NoOp
+ if (cc.getClusterEventProducer() instanceof DelegatingClusterEventProducer) {
+ ((DelegatingClusterEventProducer) cc.getClusterEventProducer())
+ .setDelegate(new NoOpProducer(cc));
+ } else {
+ log.warn("Can't configure plugin-based ClusterEventProducer while CoreContainer is still loading - " +
+ " using existing implementation {}", cc.getClusterEventProducer().getClass().getName());
+ }
+ }
+ }
+
+ @Override
+ public void modified(ContainerPluginsRegistry.ApiInfo old, ContainerPluginsRegistry.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ plugins.registerListener(pluginListener);
+ created = true;
+ return clusterEventProducer;
+ }
+
+ private void transferListeners(ClusterEventProducer target, ContainerPluginsRegistry plugins) {
+ // stop capturing listener plugins
+ plugins.unregisterListener(initialPluginListener);
+ // transfer listeners that are already registered
+ listeners.forEach((type, listenersSet) -> {
+ listenersSet.forEach(listener -> target.registerListener(listener, type));
+ });
+ listeners.clear();
Review comment:
Can't this be causing issues with methods iterating over `listeners` in the base class? Maybe need to synchronize `transferListeners`?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517211904
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private ContainerPluginsRegistry.PluginRegistryListener initialPluginListener;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ super(cc);
+ initialPluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
+ @Override
+ public void added(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(ContainerPluginsRegistry.ApiInfo old, ContainerPluginsRegistry.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ @Override
+ public Set<ClusterEvent.EventType> getSupportedEventTypes() {
+ return NoOpProducer.ALL_EVENT_TYPES;
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public ContainerPluginsRegistry.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.ContainerPluginsRegistry.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public DelegatingClusterEventProducer create(ContainerPluginsRegistry plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final DelegatingClusterEventProducer clusterEventProducer = new DelegatingClusterEventProducer(cc);
+ // since this is a ClusterSingleton, register it as such
+ cc.getClusterSingletons().getSingletons().put(ClusterEventProducer.PLUGIN_NAME +"_delegate", clusterEventProducer);
+ ContainerPluginsRegistry.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer.setDelegate((ClusterEventProducer) clusterEventProducerInfo.getInstance());
+ } else {
+ // use the default NoOp impl
+ }
+ // transfer those listeners that were already registered to the initial impl
+ transferListeners(clusterEventProducer, plugins);
Review comment:
Any potential for race conditions here? Can events be produced while this code executes?
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r514282158
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
> There is no reason why we can't implement it. I think we should.
Sure, but it's not a part of this PR. If/when we add it then we can drop this initialization of the default impl.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] murblanc commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r517211398
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,173 @@
+package org.apache.solr.cluster.events.impl;
+
+import org.apache.solr.api.ContainerPluginsRegistry;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.ClusterEventProducerBase;
+import org.apache.solr.cluster.events.NoOpProducer;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.lang.invoke.MethodHandles;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory extends ClusterEventProducerBase {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private ContainerPluginsRegistry.PluginRegistryListener initialPluginListener;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ super(cc);
+ initialPluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
+ @Override
+ public void added(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(ContainerPluginsRegistry.ApiInfo old, ContainerPluginsRegistry.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ @Override
+ public Set<ClusterEvent.EventType> getSupportedEventTypes() {
+ return NoOpProducer.ALL_EVENT_TYPES;
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public ContainerPluginsRegistry.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.ContainerPluginsRegistry.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public DelegatingClusterEventProducer create(ContainerPluginsRegistry plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final DelegatingClusterEventProducer clusterEventProducer = new DelegatingClusterEventProducer(cc);
+ // since this is a ClusterSingleton, register it as such
+ cc.getClusterSingletons().getSingletons().put(ClusterEventProducer.PLUGIN_NAME +"_delegate", clusterEventProducer);
+ ContainerPluginsRegistry.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer.setDelegate((ClusterEventProducer) clusterEventProducerInfo.getInstance());
+ } else {
+ // use the default NoOp impl
+ }
+ // transfer those listeners that were already registered to the initial impl
+ transferListeners(clusterEventProducer, plugins);
+ // install plugin registry listener
+ ContainerPluginsRegistry.PluginRegistryListener pluginListener = new ContainerPluginsRegistry.PluginRegistryListener() {
+ @Override
+ public void added(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ ClusterEventListener listener = (ClusterEventListener) instance;
+ clusterEventProducer.registerListener(listener);
+ } else if (instance instanceof ClusterEventProducer) {
+ // replace the existing impl
+ if (cc.getClusterEventProducer() instanceof DelegatingClusterEventProducer) {
+ ((DelegatingClusterEventProducer) cc.getClusterEventProducer())
+ .setDelegate((ClusterEventProducer) instance);
+ } else {
+ log.warn("Can't configure plugin-based ClusterEventProducer while CoreContainer is still loading - " +
+ " using existing implementation {}", cc.getClusterEventProducer().getClass().getName());
+ }
+ }
+ }
+
+ @Override
+ public void deleted(ContainerPluginsRegistry.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ ClusterEventListener listener = (ClusterEventListener) instance;
+ clusterEventProducer.unregisterListener(listener);
+ } else if (instance instanceof ClusterEventProducer) {
+ // replace the existing impl with NoOp
+ if (cc.getClusterEventProducer() instanceof DelegatingClusterEventProducer) {
+ ((DelegatingClusterEventProducer) cc.getClusterEventProducer())
+ .setDelegate(new NoOpProducer(cc));
+ } else {
+ log.warn("Can't configure plugin-based ClusterEventProducer while CoreContainer is still loading - " +
+ " using existing implementation {}", cc.getClusterEventProducer().getClass().getName());
+ }
+ }
+ }
+
+ @Override
+ public void modified(ContainerPluginsRegistry.ApiInfo old, ContainerPluginsRegistry.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ plugins.registerListener(pluginListener);
+ created = true;
+ return clusterEventProducer;
+ }
+
+ private void transferListeners(ClusterEventProducer target, ContainerPluginsRegistry plugins) {
Review comment:
Would there be a way to register listeners into an intermediate object (not tied to any specific producer), then have event producers feed events into this object (conceptually it would behave like a queue, but the implementation can be direct listener method calls). This would save having methods such as this one (and would not require having a producer in order to register listeners, if that helps...).
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r515762968
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
NO , please.
I am -1 on enabling this by default
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
NO , please.
I am -1 on enabling this by default. We are trying to repeat the same mistake we did with the previous version. TBH, nobody should be bothered with any event producer and the user should not pay a price for that
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
NO , please.
I am -1 on enabling this by default. We are trying to repeat the same mistake we did with the previous version. TBH, nobody should be bothered with any event producer and the user should not pay a price for that unless it is explicitly configured
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#issuecomment-722317372
Merged to master.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1962:
URL: https://github.com/apache/lucene-solr/pull/1962#discussion_r512718408
##########
File path: solr/core/src/java/org/apache/solr/core/ClusterEventProducerFactory.java
##########
@@ -0,0 +1,178 @@
+package org.apache.solr.core;
+
+import org.apache.solr.api.CustomContainerPlugins;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.ClusterEventProducer;
+import org.apache.solr.cluster.events.impl.DefaultClusterEventProducer;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * This class helps in handling the initial registration of plugin-based listeners,
+ * when both the final {@link ClusterEventProducer} implementation and listeners
+ * are configured using plugins.
+ */
+public class ClusterEventProducerFactory implements ClusterEventProducer {
+ private Map<ClusterEvent.EventType, Set<ClusterEventListener>> initialListeners = new HashMap<>();
+ private CustomContainerPlugins.PluginRegistryListener initialPluginListener;
+ private final CoreContainer cc;
+ private boolean created = false;
+
+ public ClusterEventProducerFactory(CoreContainer cc) {
+ this.cc = cc;
+ initialPluginListener = new CustomContainerPlugins.PluginRegistryListener() {
+ @Override
+ public void added(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ registerListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void deleted(CustomContainerPlugins.ApiInfo plugin) {
+ if (plugin == null || plugin.getInstance() == null) {
+ return;
+ }
+ Object instance = plugin.getInstance();
+ if (instance instanceof ClusterEventListener) {
+ unregisterListener((ClusterEventListener) instance);
+ }
+ }
+
+ @Override
+ public void modified(CustomContainerPlugins.ApiInfo old, CustomContainerPlugins.ApiInfo replacement) {
+ added(replacement);
+ deleted(old);
+ }
+ };
+ }
+
+ /**
+ * This method returns an initial plugin registry listener that helps to capture the
+ * freshly loaded listener plugins before the final cluster event producer is created.
+ * @return initial listener
+ */
+ public CustomContainerPlugins.PluginRegistryListener getPluginRegistryListener() {
+ return initialPluginListener;
+ }
+
+ /**
+ * Create a {@link ClusterEventProducer} based on the current plugin configurations.
+ * <p>NOTE: this method can only be called once because it has side-effects, such as
+ * transferring the initially collected listeners to the resulting producer's instance, and
+ * installing a {@link org.apache.solr.api.CustomContainerPlugins.PluginRegistryListener}.
+ * Calling this method more than once will result in an exception.</p>
+ * @param plugins current plugin configurations
+ * @return configured instance of cluster event producer (with side-effects, see above)
+ */
+ public ClusterEventProducer create(CustomContainerPlugins plugins) {
+ if (created) {
+ throw new RuntimeException("this factory can be called only once!");
+ }
+ final ClusterEventProducer clusterEventProducer;
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = plugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ // the listener in ClusterSingletons already registered it
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ // create the default impl
+ clusterEventProducer = new DefaultClusterEventProducer(cc);
Review comment:
AFAIK there is no mechanism in plugins to require the presence of another plugin. If we decide to make this functionality an opt-in then other plugins that depend on this may silently fail to work properly because the user forgot to specify this opt-in. This problem is not specific to this plugin, but rather it's a lack of functionality in our whole plugin ecosystem.
Until we have a working mechanism to specify plugin requirements / dependencies I think we have to provide sensible defaults, and a sensible default is a working implementation, plus the ability to opt-out or change the default impl. I'll add a mechanism to opt-out.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org