[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
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 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.
+ * 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.
+ * @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 GitH
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@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
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>
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 perType = listeners.get(type);
+if (perType != null) {
+ perType.remove(listener);
+ if (perType.isEmpty()) {
+listeners.remove(type);
+ }
+}
+ }
+}
+ }
+
+ @Override
+ public State getState() {
+return state;
+ }
+
+ public Map>
getEventListeners() {
+return listeners;
+ }
+
+ public CoreContainer getCoreContainer() {
+return cc;
+ }
+
+ public abstract Set 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:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@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
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>
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.
+ * 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.
+ * @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:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@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
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>
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.
+ * 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.
+ * @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:
us...@infra.apache.org
-
To unsubscribe, e-mail: iss
[GitHub] [lucene-solr] sigram commented on a change in pull request #1962: SOLR-14749 Provide a clean API for cluster-level event processing
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>
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.
+ * 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.
+ * @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:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@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
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:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
