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/08/17 17:35:39 UTC
[GitHub] [lucene-solr] sigram opened a new pull request #1758: SOLR-14749: Initial draft.
sigram opened a new pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758
See JIRA for details.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473848530
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
@chatman once again, this PR is NOT about the autoscaling (even though that was the original motivation). The whole point of this PR is for Solr to provide a minimal facility that allows plugins to react to cluster-wide events - and ONE particular application would be to re-implement the autoscaling triggers.
Without this API each package (autoscaling or any other package that needs it) would have to re-implement the notion of cluster-wide event monitoring and generation.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r499169806
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEvent.java
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.common.MapWriter;
+
+import java.io.IOException;
+import java.time.Instant;
+
+/**
+ * Cluster-level event.
+ */
+public interface ClusterEvent extends MapWriter {
Review comment:
Makes sense, I'll remove 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] murblanc commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491887454
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducer.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.cloud.ClusterSingleton;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Component that produces {@link ClusterEvent} instances.
+ */
+public interface ClusterEventProducer extends ClusterSingleton {
+
+ String PLUGIN_NAME = "clusterEventProducer";
+
+ /**
+ * Returns a modifiable map of event types and listeners to process events
+ * of a given type.
+ */
+ Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners();
Review comment:
These implementations will be internal (the `ClusterEvent` values are a finite defined set so any new events imply changes to internal implementation), so we likely can accept to use `EnumMap` in these.
Potential efficiency gain vs. flexibility?
I would have used `EnumMap` but ok to keep `Map`.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r494131970
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -889,7 +896,37 @@ public void load() {
ContainerPluginsApi containerPluginsApi = new ContainerPluginsApi(this);
containerHandlers.getApiBag().registerObject(containerPluginsApi.readAPI);
containerHandlers.getApiBag().registerObject(containerPluginsApi.editAPI);
+
+ // create the ClusterEventProducer
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = customContainerPlugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ clusterEventProducer = new ClusterEventProducerImpl(this);
+ }
+ // init ClusterSingleton-s
+ Map<String, ClusterSingleton> singletons = new ConcurrentHashMap<>();
+ if (clusterEventProducer instanceof ClusterSingleton) {
+ singletons.put(ClusterEventProducer.PLUGIN_NAME, (ClusterSingleton) clusterEventProducer);
+ }
+
+ // register ClusterSingleton handlers
+ // XXX register also other ClusterSingleton-s from packages - how?
+ containerHandlers.keySet().forEach(handlerName -> {
Review comment:
The purpose of this code is to build a registry of existing `ClusterSingleton` implementations (perhaps this should go to a dedicated registry class). We don't have a dependency injection framework, so we need to somewhere perform the discovery and registration ourselves.
And we need a registry in order to manage the `ClusterSingleton` lifecycle together with the Overseer leader life-cycle.
----------------------------------------------------------------
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] chatman commented on pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-701962594
I see that things like these are getting added to solr-core:
CollectionsAddedEvent
CollectionsRemovedEvent
NodesDownEvent
ReplicasDownEvent
ClusterPropertiesChangedEvent
Reminded me of "Lousiana is expecting a rain event": https://www.youtube.com/watch?v=vdPy5Ikn7dw
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472270151
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Sorry, this was asked here already: https://github.com/apache/lucene-solr/pull/1758#discussion_r471838373
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491870172
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducer.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.cloud.ClusterSingleton;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Component that produces {@link ClusterEvent} instances.
+ */
+public interface ClusterEventProducer extends ClusterSingleton {
+
+ String PLUGIN_NAME = "clusterEventProducer";
+
+ /**
+ * Returns a modifiable map of event types and listeners to process events
+ * of a given type.
+ */
+ Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners();
Review comment:
`EnumMap` is a concrete class, if we specify just a `Map` here then implementations are free to use whatever impl they want.
----------------------------------------------------------------
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 pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-675173406
generic suggestion.
Please get rid of "cloud" in names. Here the package name can be changed to `o.a.s.cluster.events`
`cloud` was definitely a bad choice for a name. Let's get rid of that in the new APIs
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r490279251
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/CollectionsAddedEvent.java
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Collection;
+
+/**
+ * Event generated when some collections have been added.
+ */
+public interface CollectionsAddedEvent extends ClusterEvent {
+
+ @Override
+ default EventType getType() {
+ return EventType.COLLECTIONS_ADDED;
+ }
+
+ Collection<String> getCollectionNames();
Review comment:
Shall we move this to an iterator model?
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r498982949
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/CollectionsRepairEventListener.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events.impl;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.cloud.SolrCloudManager;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.api.collections.Assign;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.NodesDownEvent;
+import org.apache.solr.cluster.events.ReplicasDownEvent;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.ReplicaPosition;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This is an illustration how to re-implement the combination of 8x
+ * NodeLostTrigger and AutoAddReplicasPlanAction to maintain the collection's replication factor.
+ * <p>NOTE: there's no support for 'waitFor' yet.</p>
+ * <p>NOTE 2: this functionality would be probably more reliable when executed also as a
+ * periodically scheduled check - both as a reactive (listener) and proactive (scheduled) measure.</p>
+ */
+public class CollectionsRepairEventListener implements ClusterEventListener {
Review comment:
Is this class registered somewhere to be actually called? If so I missed that part.
----------------------------------------------------------------
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 #1758: SOLR-14749: Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r471656466
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/ScheduledEvent.java
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
+ String getScheduleName();
+ Object getScheduleParam(String key);
Review comment:
Can/shall we be more specific about what scheduling means? Rather than returning Object that will be cast into...
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/NodeDownEvent.java
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface NodeDownEvent extends ClusterEvent {
Review comment:
Likely can provide a defaut implementation here of getType()?
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473817554
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
> Leader rebalancing job, cluster rebalancing, removing replicas from a node to be removed, unnecessary replica removal, core corruption checking etc etc
AFAICT, these should be actions/operations that should be plugged into the autoscaling framework package/module. And, the scheduling for these should also be completely self-contained within that framework (with no traces in the solr-core). IOW, ScheduledEvent should not be a first class concept/interface in "solr-core".
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472058053
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/ScheduledEvent.java
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
+ String getScheduleName();
+ Object getScheduleParam(String key);
Review comment:
Thinking more about it, we should use a strongly-typed Schedule.
Scheduled event is something that Solr generates on a predefined schedule ;) It's basically a general-purpose scheduler that is available via this API. There's a need for some periodic maintenance tasks, and currently it was implemented as a part of the autoscaling triggers (ScheduledTrigger, which in turn invoked eg. inactive shard cleanup and collection repair tasks).
I'm open to other suggestions - we could make it a separate API, but modelling it as a cluster event has benefits too (uniform way to register for and to process events, and it avoids adding yet another 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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472263584
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
I'm not sure Solr core code should be responsible for coordinating plugins' coordination like this. I feel we should keep this coordination left to the plugins. I am NOT saying that the autoscaling plugin should do this on its own, but I am just saying that "some plugin" should do it for the autoscaling plugin. IOW, this coordination code shouldn't be in solr-core.
As an example, we can have two plugins:
"autoscaling-framework-plugin" depends on "lifecycle-coordination-plugin". The latter can have these concepts defined in them, including the actual implementation.
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Can you please provide an example of a scheduled event? I am unable to see why a scheduled event is needed in Solr core.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r490294923
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterPropertiesChangedEvent.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Map;
+
+/**
+ * Event generated when {@link org.apache.solr.common.cloud.ZkStateReader#CLUSTER_PROPS} is modified.
+ */
+public interface ClusterPropertiesChangedEvent extends ClusterEvent {
+
+ @Override
+ default EventType getType() {
+ return EventType.CLUSTER_PROPERTIES_CHANGED;
+ }
+
+ Map<String, Object> getOldClusterProperties();
Review comment:
Is it really the role of the event to provide the old vs new properties? I'd prefer the event to provide the change only, let the subscriber fetch the new state if it wants to assemble the before/after state (and possibly the listener is keeping its own copy of what the old state is, and that old state is not necessarily equal to the old state as known by the change event implementation).
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r498957288
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducer.java
##########
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.cloud.ClusterSingleton;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Component that produces {@link ClusterEvent} instances.
+ */
+public interface ClusterEventProducer extends ClusterSingleton {
+
+ String PLUGIN_NAME = "clusterEventProducer";
+
+ default String getName() {
+ return PLUGIN_NAME;
+ }
+
+ /**
+ * Returns a modifiable map of event types and listeners to process events
+ * of a given type.
+ */
+ Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners();
+
+ /**
+ * Register an event listener for processing the specified event types.
+ * @param listener non-null listener. If the same instance of the listener is
+ * already registered it will be ignored.
+ * @param eventTypes non-empty array of event types that this listener
+ * is being registered for. If this is null or empty then all types will be used.
+ */
+ default void registerListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) throws Exception {
+ Objects.requireNonNull(listener);
+ if (eventTypes == null || eventTypes.length == 0) {
+ eventTypes = ClusterEvent.EventType.values();
+ }
+ for (ClusterEvent.EventType type : eventTypes) {
+ Set<ClusterEventListener> perType = getEventListeners().computeIfAbsent(type, t -> ConcurrentHashMap.newKeySet());
+ perType.add(listener);
+ }
+ }
+
+ /**
+ * Unregister an event listener.
+ * @param listener non-null listener.
+ */
+ default void unregisterListener(ClusterEventListener listener) {
+ Objects.requireNonNull(listener);
+ getEventListeners().forEach((type, listeners) -> {
+ listeners.remove(listener);
+ });
+ }
+
+ /**
+ * Unregister an event listener for specified event types.
+ * @param listener non-null listener.
+ * @param eventTypes event types from which the listener will be unregistered. If this
+ * is null or empty then all event types will be used
+ */
+ default void unregisterListener(ClusterEventListener listener, ClusterEvent.EventType... eventTypes) {
+ Objects.requireNonNull(listener);
+ if (eventTypes == null || eventTypes.length == 0) {
+ eventTypes = ClusterEvent.EventType.values();
+ }
+ for (ClusterEvent.EventType type : eventTypes) {
+ getEventListeners()
+ .getOrDefault(type, Collections.emptySet())
+ .remove(listener);
+ }
+ }
+
+ /**
+ * Fire an event. This method will call registered listeners that subscribed to the
+ * type of event being passed.
+ * @param event cluster event
+ */
+ default void fireEvent(ClusterEvent event) {
Review comment:
Firing (publishing) events is definitely needed in solr core where events are created.
Do we need to couple the publishing of events and managing the consumption of events as done here, or maybe have a simple publishing interface (with this single method) and a separate one for implementing the even distribution logic?
This could eventually allow keeping in Solr core the things that really belong there (creating events related to solr internal state change and publishing them) while allowing to move the event bus logic outside of solr core.
----------------------------------------------------------------
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] chatman commented on pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-701966468
> IMHO ClusterEventListener-s make sense only if they are also ClusterSingleton-s
So, does this mean that ClusterSingleton is the trojan horse with which come so many *Event classes into solr-core? If I can convince Noble and you on not putting ClusterSingleton interface into solr-core (and instead have a separate module for that), can we get away without such classes (and potentially tons more for other Events in future) in the core?
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r496005660
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/CollectionsRepairEventListener.java
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events.impl;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.cloud.SolrCloudManager;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.api.collections.Assign;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.NodesDownEvent;
+import org.apache.solr.cluster.events.ReplicasDownEvent;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.ReplicaPosition;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This is an illustration how to re-implement the combination of 8x
+ * NodeLostTrigger and AutoAddReplicasPlanAction to maintain the collection's replication factor.
+ * <p>NOTE: there's no support for 'waitFor' yet.</p>
+ * <p>NOTE 2: this functionality would be probably more reliable when executed also as a
+ * periodically scheduled check - both as a reactive (listener) and proactive (scheduled) measure.</p>
+ */
+public class CollectionsRepairEventListener implements ClusterEventListener {
+ private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ public static final String PLUGIN_NAME = "collectionsRepairListener";
+ private static final String ASYNC_ID_PREFIX = "_async_" + PLUGIN_NAME;
+ private static final AtomicInteger counter = new AtomicInteger();
+
+ private final SolrClient solrClient;
+ private final SolrCloudManager solrCloudManager;
+
+ private boolean running = false;
Review comment:
Likely needs to be volatile (or get proper synchronization).
----------------------------------------------------------------
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 edited a comment on pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul edited a comment on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-698054014
Is it possible to explain the changes to public touch points?
* an example of how you register some type of plugin
* how it looks in some JSON in ZK
* If a plugin can be registered using a public API, is there a testcase for the same?
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473802780
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
@sigram So long as concept of "triggers" is not a first class citizen in solr-core, I'm +1 to the idea.
My understanding was that the autoscaling framework will introduce the concept of "triggers" (with no traces in solr-core) and that framework will also have first party trigger impls. Third party triggers can plug into the framework as dependent packages.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-698204377
> an example of how you register some type of plugin
Still open for suggestions. IMHO `ClusterEventListener`-s make sense only if they are also `ClusterSingleton`-s. If that's the case then there's that messy section in `CoreContainer` that already registers `ClusterSingleton`-s, and we can add a section that additionally registers any instance that implements `ClusterEventListener` with the `ClusterEventProducer`.
> how it looks in some JSON in ZK
I'm reusing the plugin-s configs from `CustomContainerPlugins` so it will look like any other plugin config.
> If a plugin can be registered using a public API, is there a testcase for the same?
Not yet :)
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472951033
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
I'm advocating for a modular approach to building this entire framework:
```
solr-core
^
|
(dependson, via this interfaces in this PR)
|
|
autoscaling-framework (1st party pkg) ----dependson--> coordination-plugin (1st party pkg) ---- dependson --> solr-core
^
|
|
murblanc-trigger-package (3rd party pacakge, if some user wants to build his/her own trigger etc.)
```
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491870338
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterPropertiesChangedEvent.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Map;
+
+/**
+ * Event generated when {@link org.apache.solr.common.cloud.ZkStateReader#CLUSTER_PROPS} is modified.
+ */
+public interface ClusterPropertiesChangedEvent extends ClusterEvent {
+
+ @Override
+ default EventType getType() {
+ return EventType.CLUSTER_PROPERTIES_CHANGED;
+ }
+
+ Map<String, Object> getOldClusterProperties();
Review comment:
Ok, makes sense.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r493975213
##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -78,6 +79,10 @@ public void writeMap(EntryWriter ew) throws IOException {
currentPlugins.forEach(ew.getBiConsumer());
}
+ public synchronized ApiInfo getPlugin(String name) {
Review comment:
why is this synchronized ?
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -889,7 +896,37 @@ public void load() {
ContainerPluginsApi containerPluginsApi = new ContainerPluginsApi(this);
containerHandlers.getApiBag().registerObject(containerPluginsApi.readAPI);
containerHandlers.getApiBag().registerObject(containerPluginsApi.editAPI);
+
+ // create the ClusterEventProducer
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = customContainerPlugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ clusterEventProducer = new ClusterEventProducerImpl(this);
+ }
+ // init ClusterSingleton-s
+ Map<String, ClusterSingleton> singletons = new ConcurrentHashMap<>();
+ if (clusterEventProducer instanceof ClusterSingleton) {
+ singletons.put(ClusterEventProducer.PLUGIN_NAME, (ClusterSingleton) clusterEventProducer);
+ }
+
+ // register ClusterSingleton handlers
+ // XXX register also other ClusterSingleton-s from packages - how?
+ containerHandlers.keySet().forEach(handlerName -> {
Review comment:
what are you trying to do here?
I'm totally confused about
- What are the objects you are registering
- where they are registered?
- what is the purpose?
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
Review comment:
you know this is a backward incompatible change , right?
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
permission = PermissionNameProvider.Name.COLL_READ_PERM)
public void list(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException {
- rsp.add(PLUGIN, plugins(zkClientSupplier));
+ rsp.add(PLUGINS, plugins(zkClientSupplier));
Review comment:
this too is backward incompatible
##########
File path: solr/core/src/java/org/apache/solr/packagemanager/PackageManager.java
##########
@@ -231,7 +232,7 @@ public void uninstall(String packageName, String version) {
}
}
@SuppressWarnings({"unchecked"})
- Map<String, Object> clusterPlugins = (Map<String, Object>) result.getOrDefault("plugin", Collections.emptyMap());
+ Map<String, Object> clusterPlugins = (Map<String, Object>) result.getOrDefault(ContainerPluginsApi.PLUGINS, Collections.emptyMap());
Review comment:
backward incompatble
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472343024
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
If you don't provude thus level of servuce to plugins (single instance running) you force them to somehow do it on their own. How would they do that? I'm strongly against exposing internal implementation "details" such as ZK. Do we let plugins open
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r494131970
##########
File path: solr/core/src/java/org/apache/solr/core/CoreContainer.java
##########
@@ -889,7 +896,37 @@ public void load() {
ContainerPluginsApi containerPluginsApi = new ContainerPluginsApi(this);
containerHandlers.getApiBag().registerObject(containerPluginsApi.readAPI);
containerHandlers.getApiBag().registerObject(containerPluginsApi.editAPI);
+
+ // create the ClusterEventProducer
+ CustomContainerPlugins.ApiInfo clusterEventProducerInfo = customContainerPlugins.getPlugin(ClusterEventProducer.PLUGIN_NAME);
+ if (clusterEventProducerInfo != null) {
+ clusterEventProducer = (ClusterEventProducer) clusterEventProducerInfo.getInstance();
+ } else {
+ clusterEventProducer = new ClusterEventProducerImpl(this);
+ }
+ // init ClusterSingleton-s
+ Map<String, ClusterSingleton> singletons = new ConcurrentHashMap<>();
+ if (clusterEventProducer instanceof ClusterSingleton) {
+ singletons.put(ClusterEventProducer.PLUGIN_NAME, (ClusterSingleton) clusterEventProducer);
+ }
+
+ // register ClusterSingleton handlers
+ // XXX register also other ClusterSingleton-s from packages - how?
+ containerHandlers.keySet().forEach(handlerName -> {
Review comment:
The purpose of this code is to build a registry of existing `ClusterSingleton` implementations (perhaps this should go to a dedicated registry class). We don't have a dependency injection framework, so we need to somewhere perform the discovery and registration ourselves.
And we need a registry in order to manage the `ClusterSingleton` lifecycle together with the Overseer leader life-cycle.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
Review comment:
Right, but this is for 9.0 so we can break back-compat if it's justified - and I think it is because the singular name here doesn't make sense, as it is a location where multiple plugin configurations are defined.
In any case we can provide a back-compat shim for 9.0 to also accept `plugin` singular.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
permission = PermissionNameProvider.Name.COLL_READ_PERM)
public void list(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException {
- rsp.add(PLUGIN, plugins(zkClientSupplier));
+ rsp.add(PLUGINS, plugins(zkClientSupplier));
Review comment:
See above.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-712195659
I'm closing this PR - other PRs contain all the code here, only split into parts (events API, ClusterSingleton and scheduler)
----------------------------------------------------------------
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] janhoy commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473907204
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Is the package system mature enough to handle a dependency graph? I.e 3rd party pks A ---> 1st party pkg B ---> 1st party package C? Would attempt to install A give an error that B must be installed first? Or would install of A trigger auto install of (1st party) pkg B?
I'd prefer to not go there yet, and let packages depend on core APIs only as of now.
We could perhaps make schedulers pluggable, and only include a very slim `TimeIntervalScheduler` in core, and let plugins add their more complex schedulers such as `CronScheduler` as part of the package if needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-694280735
Would love to see as part of this PR an example of an event listener doing something (remotely) useful, to get a better feel of the whole process. That sample code could be removed later when "real" code doing event listening is introduced.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r484517789
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
> For some context, almost all devops engineers I've interacted with do not trust Solr to carry out these things in an unsupervised manner
IIUIC this objection was to the scheduler part. This is now removed from this PR - changes here are related purely to the facility to generate cluster-state related events and the API for registering listeners to these events.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473052101
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
Ok, I agree about keeping module dependencies clean. Didn't realize "Solr-core" meant just a module in the project.
We do need some code in `solr-core` though, at least interfaces to be able to interact with other modules without depending on them (+ dependency injection or configuration to instantiate actual concrete classes).
I believe that understanding what's really needed happens when the interfaces and the configuration is wired into Solr in some place or another, then we should move out of solr-core everything left there without reason (thinking of [PR 1684](https://github.com/apache/lucene-solr/pull/1684/files) as I write this comment, likely the `impl `and `plugin `packages there can be moved out of `solr-core`).
----------------------------------------------------------------
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] janhoy commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473907204
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Is the package system mature enough to handle a dependency graph? I.e `3rd party pks A ---> 1st party pkg B ---> solr-core`? Would attempt to install A give an error that B must be installed first? Or would install of A trigger auto install of (1st party) pkg B?
I'd prefer to not go there yet, and let packages depend on core APIs only as of now.
We could perhaps make schedulers pluggable, and only include a very slim `TimeIntervalScheduler` in core, and let plugins add their more complex schedulers such as `CronScheduler` as part of the package if needed?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473848530
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
@chatman once again, this PR is NOT only about the autoscaling (even though that was the original motivation). The whole point of this PR is for Solr to provide a minimal facility that allows plugins to react to cluster-wide events - and ONE particular application would be to re-implement the autoscaling triggers.
Without this API each package (autoscaling or any other package that needs it) would have to re-implement the notion of cluster-wide event monitoring and generation.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473828077
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
There is nothing wrong in making all these additions in a module.
Please not that this module is a part of Solr. it will be shipped with Solr and the user may just have to run a command to enable that
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472053144
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/NodeDownEvent.java
##########
@@ -0,0 +1,24 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface NodeDownEvent extends ClusterEvent {
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] murblanc commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473959273
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
Another general (semi lame) comment from me: "good practices" are good guidelines but we also have to be pragmatic/realistic. To some extent it means understanding the big picture of how a (new) feature needs to be implemented and then seeing how that plays with the principles of separation etc we all want. Sometimes it doesn't play well (due to maturity of the rest of the code or other reasons), and possibly during design phases we didn't anticipate all the touchpoints.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r497515320
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
Review comment:
We can , but that doesn't mean we should. This is just a cosmetic change. Adds no value to user but causes enormous pain to the user. We just make people not upgrade at all
I'm -1 on doing a breaking change
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r471838373
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/ScheduledEvent.java
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
+ String getScheduleName();
+ Object getScheduleParam(String key);
Review comment:
What is a Scheduled event?
What is a "scheduled-param" ?
----------------------------------------------------------------
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 edited a comment on pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram edited a comment on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-688426745
FWIW, the scheduler part is now in PR #1831.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r494134314
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
Review comment:
Right, but this is for 9.0 so we can break back-compat if it's justified - and I think it is because the singular name here doesn't make sense, as it is a location where multiple plugin configurations are defined.
In any case we can provide a back-compat shim for 9.0 to also accept `plugin` singular.
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
##########
@@ -64,15 +64,15 @@ public ContainerPluginsApi(CoreContainer coreContainer) {
public class Read {
@EndPoint(method = METHOD.GET,
- path = "/cluster/plugin",
+ path = "/cluster/plugins",
permission = PermissionNameProvider.Name.COLL_READ_PERM)
public void list(SolrQueryRequest req, SolrQueryResponse rsp) throws IOException {
- rsp.add(PLUGIN, plugins(zkClientSupplier));
+ rsp.add(PLUGINS, plugins(zkClientSupplier));
Review comment:
See above.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472435099
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/ScheduledEvent.java
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
+ String getScheduleName();
+ Object getScheduleParam(String key);
Review comment:
@chatman even in 8x the scheduled events were used for things other than autoscaling, namely for inactive shard maintenance. Solr still needs this functionality in order to avoid implementing it over and over again in individual plugins.
I don't have a strong opinion whether it must be a part of the event API here, but it seems to me that it's convenient to model scheduled events in this uniform way.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473942456
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Even if package manager doesn't support dependencies today, this can still be loaded from contrib modules (that produces separate jar) that depend on reach other. Modularization is the first step towards package manager based loading. If we start with everything in solr core, then it will be considerably harder to make them into packages.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473852648
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
> plugged into the autoscaling framework
This PR is NOT just about the autoscaling. The goal is to define a generic API for cluster-level events. Autoscaling plugins will be just one consumer of this API.
> IOW, ScheduledEvent should not be a first class concept/interface in "solr-core"
I disagree. Scheduling various maintenance tasks is a common need (not just in autoscaling), and it fits with the event API. Sure, we can define a separate API just for scheduling, but it would be still something that belongs to solr-core.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-696057300
@murblanc See `CollectionsRepairEventListener` for an example. Also the `ClusterEventProducerTest` unit test.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r490285588
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventListener.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Set;
+
+/**
+ * Components that want to be notified of cluster-wide events should use this.
+ *
+ * XXX should this work only for ClusterSingleton-s? some types of events may be
+ * XXX difficult (or pointless) to propagate to every node.
+ */
+public interface ClusterEventListener {
+
+ /**
+ * The types of events that this listener can process.
+ */
+ Set<ClusterEvent.EventType> getEventTypes();
Review comment:
Similarly, it might be useful to unregister a listener on some event types but not all...
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472948399
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
I think you're totally misunderstanding what I'm saying here.
> I'd rather have such code be in lucene-solr and not be loaded when not in use,
I'm not opposed to such code staying in official Solr repository. Even the autoscaling framework will be there. But, I am opposed to having such coordination in "solr-core" module of the repository. It should be a separate package/jar, that depends on "solr-core".
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r498954021
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEvent.java
##########
@@ -0,0 +1,57 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.common.MapWriter;
+
+import java.io.IOException;
+import java.time.Instant;
+
+/**
+ * Cluster-level event.
+ */
+public interface ClusterEvent extends MapWriter {
Review comment:
The `MapWriter` related machinery (including the `writeMap` method) doesn't belong in the interfaces IMO. It should be hidden in the implementation if it's needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-696057300
@murblanc See `CollectionsRepairEventListener` for an example. Also the `ClusterEventProducerTest` unit test.
----------------------------------------------------------------
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 pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-698054014
Is it possible to explain the changes to public touch points?
* an example of how you register some type of plugin
* how it looks in some JSON in ZK
* If a plugin can be registered , using a public API, is there a testcase for the same?
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472343024
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
If you don't provide this level of service to plugins (single instance running) you force them to somehow do it on their own. How would they even start to do that? I'm strongly against exposing internal implementation "details" such as ZK. Do we let plugins open TCP ports? Have their own ZK? Tomorrow we might decide to run plug-in on other containers/VM's that are not nodes or replace ZK by a DB. Will plugins have to reimplement another leader election or similar?
It is much cleaner and simpler to do it once in Solr.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472346084
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Leader rebalancing job, cluster rebalancing, removing replicas from a node to be removed, unnecessary replica removal, core corruption checking etc etc
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491869081
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventListener.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Set;
+
+/**
+ * Components that want to be notified of cluster-wide events should use this.
+ *
+ * XXX should this work only for ClusterSingleton-s? some types of events may be
+ * XXX difficult (or pointless) to propagate to every node.
+ */
+public interface ClusterEventListener {
+
+ /**
+ * The types of events that this listener can process.
+ */
+ Set<ClusterEvent.EventType> getEventTypes();
Review comment:
Ok.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473682999
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
@chatman this event API originated from the "triggers" API in 8x but can equally well be used outside the autoscaling - so in the above diagram it doesn't depend on autoscaling, it's adjacent to 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] murblanc commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472343024
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
If you don't provide thus level of service to plugins (single instance running) you force them to somehow do it on their own. How would they even start to do that? I'm strongly against exposing internal implementation "details" such as ZK. Do we let plugins open TCP ports? Have their own ZK? Tomorrow we might decide to run plug-in on other containers/VM's that are not nodes. Will plugins have to reimplement another leader election or similar?
It is much cleaner and simpler to do it once in Solr.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472951033
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
I'm advocating for a modular approach to building this entire framework:
`autoscaling-framework (1st party pkg) ----dependson--> coordination-plugin (1st party pkg) ---- dependson --> solr-core
^
|
|
murblanc-trigger-package (3rd party pacakge)
`
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r490288384
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducer.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.cloud.ClusterSingleton;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Component that produces {@link ClusterEvent} instances.
+ */
+public interface ClusterEventProducer extends ClusterSingleton {
+
+ String PLUGIN_NAME = "clusterEventProducer";
+
+ /**
+ * Returns a modifiable map of event types and listeners to process events
+ * of a given type.
+ */
+ Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners();
Review comment:
What about using an `EnumMap` here instead?
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473852648
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
> plugged into the autoscaling framework
This PR is NOT just about the autoscaling. The goal is to define a generic API for cluster-level events. Autoscaling plugins will be just one consumer of this API.
> IOW, ScheduledEvent should not be a first class concept/interface in "solr-core"
I disagree. Scheduling various maintenance tasks is a common need (not just in autoscaling), and it fits with the event API. Sure, we can define a separate API just for scheduling, but it would be still something that belongs to solr-core.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491867642
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/CollectionsAddedEvent.java
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Collection;
+
+/**
+ * Event generated when some collections have been added.
+ */
+public interface CollectionsAddedEvent extends ClusterEvent {
+
+ @Override
+ default EventType getType() {
+ return EventType.COLLECTIONS_ADDED;
+ }
+
+ Collection<String> getCollectionNames();
Review comment:
Ok.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-688426745
FWIW, the scheduler part is now in PR 1831.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472951033
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
I'm advocating for a modular approach to building this entire framework:
```
autoscaling-framework (1st party pkg) ----dependson--> coordination-plugin (1st party pkg) ---- dependson --> solr-core
^
|
|
murblanc-trigger-package (3rd party pacakge)
```
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473953526
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
First step is separating interfaces and implementation. If `solr-core` is to interact with non core modules, it needs a way to talk to them. After interfaces comes the question of instantiation or dependency injection into `solr-core`.
Even if package selection/install is not automated, having the right code for a clean separation will make evolution much easier.
To some extent, if `solr-core` doesn't need to talk (directly or indirectly) to another module, then maybe the module doesn't belong at all in Solr?
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473884994
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
> Leader rebalancing job, cluster rebalancing, removing replicas from a node to be removed, unnecessary replica removal, core corruption checking etc etc
For some context, almost all devops engineers I've interacted with do not trust Solr to carry out these things in an unsupervised manner because of potential disruption to stability because of these "maintenance operations".
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
muse-dev[bot] commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r498037436
##########
File path: solr/core/src/java/org/apache/solr/api/CustomContainerPlugins.java
##########
@@ -163,6 +174,47 @@ public synchronized void refresh() {
}
}
+ private void handleClusterSingleton(ApiInfo newApiInfo, ApiInfo oldApiInfo) {
+ if (newApiInfo != null) {
+ // register new api
+ Object instance = newApiInfo.getInstance();
+ if (instance instanceof ClusterSingleton) {
+ ClusterSingleton singleton = (ClusterSingleton) instance;
+ coreContainer.getClusterSingletons().getSingletons().put(singleton.getName(), singleton);
+ // easy check to see if we should immediately start this singleton
+ if (coreContainer.getClusterEventProducer() != null &&
+ coreContainer.getClusterEventProducer().isRunning()) {
+ try {
+ singleton.start();
+ } catch (Exception exc) {
+ log.warn("Exception starting ClusterSingleton {}: {}", newApiInfo, exc);
+ }
+ }
+ }
+ if (instance instanceof ClusterEventListener) {
+ // XXX nocommit obtain a list of supported event types from the config
+ ClusterEvent.EventType[] types = ClusterEvent.EventType.values();
+ try {
+ coreContainer.getClusterEventProducer().registerListener((ClusterEventListener) instance, types);
+ } catch (Exception exc) {
+ log.warn("Exception adding ClusterEventListener {}: {}", newApiInfo, exc);
+ }
+ }
+ }
+ if (oldApiInfo != null) {
+ // stop & unregister the old api
+ Object instance = oldApiInfo.getInstance();
+ if (instance instanceof ClusterSingleton) {
+ ClusterSingleton singleton = (ClusterSingleton) instance;
+ singleton.stop();
+ coreContainer.getClusterSingletons().getSingletons().remove(singleton.getName());
+ }
+ if (instance instanceof ClusterEventListener) {
+ coreContainer.getClusterEventProducer().unregisterListener((ClusterEventListener) instance);
Review comment:
*NULL_DEREFERENCE:* object returned by `CustomContainerPlugins.coreContainer.getClusterEventProducer()` could be null and is dereferenced at line 213.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472269946
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/ScheduledEvent.java
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
+ String getScheduleName();
+ Object getScheduleParam(String key);
Review comment:
> There's a need for some periodic maintenance tasks, and currently it was implemented as a part of the autoscaling triggers
I'm -1 on having the concept of triggers or scheduled events leaking into Solr core. It should be a concept internal to the autoscaling-framework.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473815049
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
> then we should move out of solr-core everything left there without reason
+1 Ilan. I'm fine with having this draft define interfaces in solr-core as we iterate, so long as we move it out to a separate module/package later. I felt having such clarity upfront as to what lives where would be better, but I defer to Andrzej's and your judgement for now for this draft/prototype.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473052101
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
Ok, I agree about keeping module dependencies clean. Didn't realize "Solr-core" meant just a module in the project.
We do need some code in `solr-core` though, at least interfaces to be able to interact with other modules without depending on them (+ dependency injection or configuration to instantiate actual concrete classes).
I believe that understanding what's really needed happens when the interfaces and the configuration is wired into Solr in some place or another, then we should move out of solr-core everything left there without reason (thinking of [PR 1684](https://github.com/apache/lucene-solr/pull/1684/files) as I write this comment, likely the impl and plugin packages there can be moved out of solr-core.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473881086
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
I disagree that scheduling should be a first class concept in Solr. Users/devops can (and currently do) use their own schedulers to carry out maintenance tasks for Solr. But, if Solr can provide this functionality as a "scheduler-module" or "scheduler-plugin", I am fine. But, I see no reason why scheduling should be a solr-core concept.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r490284824
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventListener.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Set;
+
+/**
+ * Components that want to be notified of cluster-wide events should use this.
+ *
+ * XXX should this work only for ClusterSingleton-s? some types of events may be
+ * XXX difficult (or pointless) to propagate to every node.
+ */
+public interface ClusterEventListener {
+
+ /**
+ * The types of events that this listener can process.
+ */
+ Set<ClusterEvent.EventType> getEventTypes();
Review comment:
Why have event types listened to as a listener attribute rather than as a parameter passed upon registration?
Even types part of the listener make it harder to reuse listeners. For example a listener that logs the event and that can be registered wherever needed (this model would force subclassing it each time).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491887454
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducer.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.cloud.ClusterSingleton;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Component that produces {@link ClusterEvent} instances.
+ */
+public interface ClusterEventProducer extends ClusterSingleton {
+
+ String PLUGIN_NAME = "clusterEventProducer";
+
+ /**
+ * Returns a modifiable map of event types and listeners to process events
+ * of a given type.
+ */
+ Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners();
Review comment:
These implementations will be internal (the `ClusterEvent` values are a finite defined set so any new events imply changes to internal implementation), so we likely can accept to use `EnumMap` in these.
Potential efficiency gain vs. flexibility?
I would have used `EnumMap` but ok to keep `Map`.
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472938739
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
This level of service can be provided to plugins, but this doesn't need to be in solr-core. This coordination can be a separate plugin outside solr-core that the autoscaling framework (or other plugins) can use.
> It is much cleaner and simpler to do it once in Solr.
Not in Solr-core, though. If user chooses to not use autoscaling framework, there should be absolutely no footprint of such a coordination layer for that user.
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
This level of service can be provided to plugins, but this doesn't need to be in solr-core. This coordination can be a separate plugin outside solr-core that the autoscaling framework (or other plugins) can use.
> It is much cleaner and simpler to do it once in Solr.
Not in Solr-core, though. If user chooses to not use autoscaling framework, there should be absolutely no footprint of such a coordination layer for that user.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472946169
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
My fear is everything that depends on the internal implementation of Solr and that's not compiled with Solr from the lucene-solr git repo makes our life harder (refactor breaks things).
I'd rather have such code be in lucene-solr and not be loaded when not in use, even though its presence in the running JVM if it's not called is really not an issue. We have loads of loaded but unused code in a running instance (think 3rd party libraries).
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472239057
##########
File path: solr/core/src/java/org/apache/solr/cloud/events/ScheduledEvent.java
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cloud.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
+ String getScheduleName();
+ Object getScheduleParam(String key);
Review comment:
I'm all for these events to be just like the other ones. Just define what a schedule is and strong type whatever can be...
----------------------------------------------------------------
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] chatman commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
chatman commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472951033
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
I'm advocating for a modular approach to building this entire framework:
```
solr-core
^
|
(dependson, via this interfaces in this PR)
|
|
autoscaling-framework (1st party pkg) ----dependson--> coordination-plugin (1st party pkg) ---- dependson --> solr-core
^
|
|
murblanc-trigger-package (3rd party pacakge)
```
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-698204377
> an example of how you register some type of plugin
Still open for suggestions. IMHO `ClusterEventListener`-s make sense only if they are also `ClusterSingleton`-s. If that's the case then there's that messy section in `CoreContainer` that already registers `ClusterSingleton`-s, and we can add a section that additionally registers any instance that implements `ClusterEventListener` with the `ClusterEventProducer`.
> how it looks in some JSON in ZK
I'm reusing the plugin-s configs from `CustomContainerPlugins` so it will look like any other plugin config.
> If a plugin can be registered using a public API, is there a testcase for the same?
Not yet :)
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r499170285
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/impl/CollectionsRepairEventListener.java
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events.impl;
+
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.solr.client.solrj.SolrClient;
+import org.apache.solr.client.solrj.cloud.SolrCloudManager;
+import org.apache.solr.client.solrj.request.CollectionAdminRequest;
+import org.apache.solr.cloud.api.collections.Assign;
+import org.apache.solr.cluster.events.ClusterEvent;
+import org.apache.solr.cluster.events.ClusterEventListener;
+import org.apache.solr.cluster.events.NodesDownEvent;
+import org.apache.solr.cluster.events.ReplicasDownEvent;
+import org.apache.solr.common.cloud.ClusterState;
+import org.apache.solr.common.cloud.Replica;
+import org.apache.solr.common.cloud.ReplicaPosition;
+import org.apache.solr.core.CoreContainer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * This is an illustration how to re-implement the combination of 8x
+ * NodeLostTrigger and AutoAddReplicasPlanAction to maintain the collection's replication factor.
+ * <p>NOTE: there's no support for 'waitFor' yet.</p>
+ * <p>NOTE 2: this functionality would be probably more reliable when executed also as a
+ * periodically scheduled check - both as a reactive (listener) and proactive (scheduled) measure.</p>
+ */
+public class CollectionsRepairEventListener implements ClusterEventListener {
Review comment:
Not yet - there's a unit test. I implemented this class to illustrate that the events API is sufficient to bring back the "autoAddReplicas" functionality that we removed in 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] murblanc commented on a change in pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r473052101
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterSingleton.java
##########
@@ -0,0 +1,14 @@
+package org.apache.solr.cluster.events;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+
+/**
+ * Intended for {@link org.apache.solr.core.CoreContainer} plugins that should be
+ * enabled only one instance per cluster.
+ * <p>Implementation detail: currently these plugins are instantiated on the
+ * Overseer leader, and closed when the current node loses its leadership.</p>
+ */
+@Retention(RetentionPolicy.RUNTIME)
+public @interface ClusterSingleton {
Review comment:
Ok, I agree about keeping module dependencies clean. Didn't realize "Solr-core" meant just a module in the project.
We do need some code in `solr-core` though, at least interfaces to be able to interact with other modules without depending on them (+ dependency injection or configuration to instantiate actual concrete classes).
I believe that understanding what's really needed happens when the interfaces and the configuration is wired into Solr in some place or another, then we should move out of solr-core everything left there without reason (thinking of [PR 1684](https://github.com/apache/lucene-solr/pull/1684/files) as I write this comment, likely the impl and plugin packages there can be moved out of `solr-core`).
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r472436918
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ScheduledEvent.java
##########
@@ -0,0 +1,30 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+/**
+ *
+ */
+public interface ScheduledEvent extends ClusterEvent {
Review comment:
Right, generally speaking any maintenance or optimization task that needs to be periodically invoked. Again, we could define it as a separate API but I think it fits the event model nicely.
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram commented on a change in pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#discussion_r491867642
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/CollectionsAddedEvent.java
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Collection;
+
+/**
+ * Event generated when some collections have been added.
+ */
+public interface CollectionsAddedEvent extends ClusterEvent {
+
+ @Override
+ default EventType getType() {
+ return EventType.COLLECTIONS_ADDED;
+ }
+
+ Collection<String> getCollectionNames();
Review comment:
Ok.
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventListener.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Set;
+
+/**
+ * Components that want to be notified of cluster-wide events should use this.
+ *
+ * XXX should this work only for ClusterSingleton-s? some types of events may be
+ * XXX difficult (or pointless) to propagate to every node.
+ */
+public interface ClusterEventListener {
+
+ /**
+ * The types of events that this listener can process.
+ */
+ Set<ClusterEvent.EventType> getEventTypes();
Review comment:
Ok.
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterEventProducer.java
##########
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import org.apache.solr.cloud.ClusterSingleton;
+
+import java.util.Collections;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Component that produces {@link ClusterEvent} instances.
+ */
+public interface ClusterEventProducer extends ClusterSingleton {
+
+ String PLUGIN_NAME = "clusterEventProducer";
+
+ /**
+ * Returns a modifiable map of event types and listeners to process events
+ * of a given type.
+ */
+ Map<ClusterEvent.EventType, Set<ClusterEventListener>> getEventListeners();
Review comment:
`EnumMap` is a concrete class, if we specify just a `Map` here then implementations are free to use whatever impl they want.
##########
File path: solr/core/src/java/org/apache/solr/cluster/events/ClusterPropertiesChangedEvent.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.cluster.events;
+
+import java.util.Map;
+
+/**
+ * Event generated when {@link org.apache.solr.common.cloud.ZkStateReader#CLUSTER_PROPS} is modified.
+ */
+public interface ClusterPropertiesChangedEvent extends ClusterEvent {
+
+ @Override
+ default EventType getType() {
+ return EventType.CLUSTER_PROPERTIES_CHANGED;
+ }
+
+ Map<String, Object> getOldClusterProperties();
Review comment:
Ok, makes sense.
----------------------------------------------------------------
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 pull request #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
murblanc commented on pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758#issuecomment-702014402
The way I look at this: can code other than Solr Core know what happens within Solr core to generate the corresponding events? I would tend to say no. I'm not talking about implementation shortcuts (Solr core internal DB a.k.a. Zookeeper being accessible to the world and can be used to generate some of the events) but fundamentally what state changes do these events (or some of these events) represent and where does that state live?
----------------------------------------------------------------
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 #1758: SOLR-14749: Provide a clean API for cluster-level event processing, Initial draft.
Posted by GitBox <gi...@apache.org>.
sigram closed pull request #1758:
URL: https://github.com/apache/lucene-solr/pull/1758
----------------------------------------------------------------
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