SOLR-14680: Provide simple interfaces to our cloud classes (only API) #1694

noblepaul · 2020-07-26T00:20:20Z

A few notes before anyone who starts reviewing this

This was created after I saw similar attempt as a part SOLR-14613: strongly typed placement plugin interface and implementation #1684 . I believe this has to receive a more wider input and review irrespective of whether devs are interested in autoscaling or not
This is a WIP PR
The concrete implementations are for demo purposes. Can be omitted, if required. Anything outside the o.a.s.cluster.api package is optional and will be removed
The interfaces are designed to be minimal to avoid overload. We can and will add more methods later. Let's not add a lot

murblanc · 2020-07-26T14:25:12Z

Can we start simple by defining the external interface without the required changes to internal classes first? Will make a smaller PR easier to discuss.

murblanc · 2020-07-26T15:03:15Z

I also feel this PR has a lot of overlap with the one I started to define an API for Autoscaling plugins (#1684).

Unless we want to pursue two distinct options for these interfaces and decide later (perfectly OK if that’s the idea) I suggest we use the other PR that has also examples of how these interfaces are used (makes it easier to reason about them and decide what goes where).

gus-asf · 2020-07-26T17:35:17Z

These interfaces look like a good way to begin. Thoughts: This is an excellent point at which to consider and then stick to a preferred set of terminology. My favorites are:

Machines are the operating system level container or hardware and have
- Nodes which are running solr processes on machines which have portions of
  - Collections which are logical indexes associated with a particular schema and have
    - Shards which split a collection on a routing value to allow scaling and have
      - Replicas which are duplicate copies implementing a shard often hosted on separate nodes.

We should strive to apply only ONE word for each level throughout the interfaces to make them as simple and easy to understand as possible.

These 5 terms all have nice clear english words that communicate their function to some extent. Core however only really means "central thing" and I have always thought it was a very confusing word to use here since it has almost no memnonic value. Slice is also good for the shard level, but I think shard is no worse and more commonly used in documentation. Although core has been used many times in the past, I don't think anyone is going to have trouble finding what they want via the terms I'm suggesting.

The Cluster level concept usually ignores Machines and speaks only of nodes... I could be convinced either way on whether or not that's a good thing.

For fun I drew a near worst case diagaram (omitting back references and convenience rollups) :)

gus-asf

I like that this specifies a read-only view. Made a comment regarding naming conventions elsewhere.

gus-asf · 2020-07-26T18:44:42Z

solr/solrj/src/java/org/apache/solr/common/cloud/sdk/SolrNode.java

+
+  String baseUrl(boolean isV2);
+
+  SimpleMap<ShardReplica> cores();


Possibly good to also answer the question of "what collections are participating in this node" and what shards of Collection X are on this node.

I would say, that is something that can be easily computed from the cores() method and can be provided as a helper method elsewhere

gus-asf · 2020-07-26T18:46:00Z

solr/solrj/src/java/org/apache/solr/common/cloud/sdk/SolrCluster.java

+ */
+public interface SolrCluster {
+  /** collections in the cluster */
+  SimpleMap<SolrCollection> collections();


Also aliases... and the Alias class returned should list the collections provided and routing info if routed. (also law of Demeter etc...)

Yes, I wanted to include aliases. Where do you propose it to be there?

I would say , There should be no Alias class/interface.

It should be as simple as
SolrCluster#collections#get(String name , boolean includeAlias)

and

SolrCollection#aliases() returns List<String>

gus-asf · 2020-07-26T18:47:24Z

solr/solrj/src/java/org/apache/solr/common/cloud/sdk/SolrCollection.java

+import org.apache.solr.common.util.SimpleMap;
+
+/** Represents a collection in Solr */
+public interface SolrCollection {


Also I've definitely had cases where I wanted a list of nodes where this collection is hosted.

That should be a utility method outside

gus-asf · 2020-07-26T18:54:12Z

solr/solrj/src/java/org/apache/solr/common/cloud/sdk/SolrCluster.java

+/**
+ * Represents a Solr cluster
+ */
+public interface SolrCluster {


Since we have our own package namespace, prepending solr isn't really needed unless we think we might also model non-solr clusters.

I know. Imagine this will also be used outside of Solr as a part of SolrJ. So if you read some client code,

Cluster cluster;

is less readable compared to

SolrCluster solrCluster;

Eventually, I would wish to replace a lot of SolrJ code/API with a standard set of interfaces.

gus-asf · 2020-07-26T18:56:16Z

solr/solrj/src/java/org/apache/solr/common/cloud/sdk/SolrCluster.java

+   * Solr node
+   */
+  String thisNode();
+


I've wondered sometimes if clusters should have name or id of some sort but that's probably another topic.

gus-asf · 2020-07-26T19:08:52Z

solr/solrj/src/java/org/apache/solr/common/util/LinkedSimpleHashMap.java

+
+import java.util.LinkedHashMap;
+
+public class LinkedSimpleHashMap<T> extends LinkedHashMap<CharSequence, T>  implements SimpleMap<T> {


this class appears unused...

yes, this was to demonstrate how to implement a default impl of SimpleMap

gus-asf · 2020-07-26T19:12:45Z

solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java

@@ -38,7 +43,7 @@
 * {@link ZkStateReader#getClusterState()}.
 * @lucene.experimental
 */
-public class ClusterState implements JSONWriter.Writable {
+public class ClusterState implements JSONWriter.Writable , SolrCluster {


Little worried that this design allows for casting of the SolrCluster reference...

We can have multiple implementations of SolrCluster .The idea of existing classes implementing these interfaces is to have readily available implementations

Let's rename Solr luster to Cluster.

murblanc · 2020-07-26T22:34:51Z

Snitches have a target (at least node or replica, but one could imagine Cluster, collection or shard if such snitches do not already exist).
Would be helpful for all these classes to implement a common interface so they can be passed when a notch target is needed in Autoscaling plugins.

(See in https://github.com/apache/lucene-solr/pull/1684/files#diff-fc8bc5eb94b9e48a24c2ec768733e72b)

chatman · 2020-07-26T23:13:31Z

As I suggested in the other PR on autoscaling, I feel we should change the package name:
#1684 (comment)

Also, WDYT about marking the concrete classes DocCollection, Replica etc. with @lucene.internal to make it clear that they shouldn't be used directly from the plugins? Alternatively, we can mark these new interfaces in this PR with a new annotation (e.g. @solr.external) that signals to plugin writers that these will be stable for their use across versions.

gus-asf · 2020-07-27T01:55:34Z

While marking things internal and external is good, a stronger possibility is to code more defensively and make "new plugins" get instantiated/injected with implementations of a facade that return instances of the interfaces. If those implementations can't just be cast to internal types we then have a much higher barrier to abuse (reflection).

As for package names I agree we need to think carefully about them. It is a question what the goal is too. If we're reworking the internals, sdk won't sound right at all. If we are building something only meant to be consumed by an outer layer of plugins to insulate the plugins from internal refactoring, something in that vein sounds fine (or maybe o.a.s.cloud.plugin.api?) Settling on what our goals are will help us decide what name is best.

noblepaul · 2020-07-27T03:04:06Z

Can we start simple by defining the external interface without the required changes to internal classes first? Will make a smaller PR easier to discuss.

Feel free to ignore the implementing classes. That was to demonstrate one way of implementing these interfaces. I'm happy to focus on the interfaces inside the o.a.s.cluster.api package

I have updated the description. Only the SDK is important. Implementation can/will be removed

noblepaul · 2020-07-27T03:06:57Z

I also feel this PR has a lot of overlap with the one I started to define an API for Autoscaling plugins #1684

Yes, totally.That ticket is trying to do something more than the scope of that ticket. I want to limit the scope of that ticket and have wider discussion on how a simple set of interfaces to represent the existing Solr cluster/cloud

noblepaul · 2020-07-27T03:10:52Z

These interfaces look like a good way to begin. Thoughts: This is an excellent point at which to consider and then stick to a preferred set of terminology. My favorites are:

There is no way to have a "machine" abstraction in Solr. We can start with a Node as an atomic unit. I wanted to carefully avoid the class names already taken up so that existing code can be left as it is

gus-asf · 2020-07-27T05:23:27Z

These interfaces look like a good way to begin. Thoughts: This is an excellent point at which to consider and then stick to a preferred set of terminology. My favorites are:

There is no way to have a "machine" abstraction in Solr. We can start of a Node as an atomic unit. I wanted to carefully avoid the class names already taken up so that existing code can be left as it is

Yeah machine is a difficult notion as it's a real world thing entirely outside java. It probably has to be optional and provided by sysprop or env var. and even so might be reducible to a property on node. Certainly detecting it would be fraught with peril. But machine designations (and rack designations) might be important to some folks.

murblanc · 2020-07-27T09:17:13Z

Many methods returning other objects are returning names of other abstractions being defined here (Shard, Collection, Node etc.).
I’d think an instance of the abstraction should be returned instead, and that instance would have a getName() method (i.e have SolrCollection getCollection() rather than String getCollection() on a Shard for example).
What’s the rationale for returning names rather than objects?

murblanc · 2020-07-27T09:34:24Z

I see two cases where we need interfaces such as defined here:

internal code. Coding to interfaces rather than the actual implementation makes for better structured code usually with less implementation leaks,
external “plugins”.

What’s the intention? If only the later, then there’s really no need to have the internal classes implement these interfaces, a wrapper is ok.
I believe addressing both points with a single interface is complicated (and to a point counterproductive as it ties internal and external views).

chatman · 2020-07-28T01:17:45Z

+1 to o.a.s.cluster.api package name. +1 to getting rid of "cloud" here.

solr/solrj/src/java/org/apache/solr/cluster/api/SolrNode.java

janhoy · 2020-07-29T14:08:29Z

solr/solrj/src/java/org/apache/solr/cluster/api/Config.java

+
+  String name();
+
+  /**set of files in the config */


General : Please format Javadocs properly with text starting on line 2 and capital letter, even if it takes more lines.

A one-line variant is acceptable: https://google.github.io/styleguide/javaguide.html#s7.1.1-javadoc-multi-line
The only problem I see with the javadoc here is that it's missing a leading space, and it should capitalize the first letter.

janhoy · 2020-07-29T14:11:35Z

solr/solrj/src/java/org/apache/solr/cluster/api/Config.java

+
+import org.apache.solr.common.SolrException;
+
+public interface Config {


Use ConfigSet instead? Config is soooo overloaded.

I don't like both names , Config & ConfigSet

configset suggests it is a set of configs? in reality it's a single configuration which has multiple files

murblanc · 2020-07-30T15:02:47Z

Machine vs Node: environment variables (rack, availability zone etc) are "machine" abstractions but can be considered at the node level. Free disk is clearly a machine abstraction (two nodes on same machine might be sharing the same free disk and skewing decisions based on it) but I don't see how this can be solved (even a single node might be sharing space with other processes).
Therefore I don't see how a machine abstraction helps.

murblanc · 2020-07-30T15:04:51Z

Would love to see sample plug-in code accessing cluster config. How the instances are obtained and how the cluster is explored (nodes, collections, shards, replicas).

noblepaul · 2020-07-31T12:53:52Z

@murblanc I shall try to provide some sample implementation in another 2-3 days

noblepaul · 2020-08-10T02:19:59Z

Opening another PR

noblepaul · 2020-08-10T11:39:10Z

I intend to merge this soon

…#1694)

…apache#1694)

noblepaul added 7 commits July 25, 2020 17:12

simple API view of Solr cluster

8afacf2

simple API view of Solr cluster

80b3ce4

simple API view of Solr cluster

a6a30eb

precommit

a6d05f8

precommit

46a371e

cleanup

001f48b

cleanup

f4fd18a

noblepaul requested a review from gus-asf July 26, 2020 00:20

noblepaul mentioned this pull request Jul 26, 2020

SOLR-14613: strongly typed placement plugin interface and implementation #1684

Closed

noblepaul added 3 commits July 26, 2020 10:38

cleanup

ba29f2e

cleanup

1eda201

precommit errors

342ae67

gus-asf reviewed Jul 26, 2020

View reviewed changes

noblepaul added 5 commits July 27, 2020 16:59

more methods

87c43bd

removed unnecessary method

ccb0ae9

removed a method

baac533

added Router

8dcd447

added ASL Header

6201980

noblepaul added 4 commits July 28, 2020 09:50

moved the package to o.a.s.cluster.api

0d39cee

added configset as well

93c5311

added exceptions

fe6aa01

added HashRange

b91b461

janhoy reviewed Jul 29, 2020

View reviewed changes

noblepaul added the clean-api label Jul 31, 2020

noblepaul added 2 commits August 3, 2020 12:05

Added replica size

0a29e4d

added the missing name() method

018c18f

noblepaul closed this Aug 10, 2020

noblepaul mentioned this pull request Aug 10, 2020

SOLR-14680: Provide an implementation for the new SolrCluster API #1730

Merged

noblepaul reopened this Aug 10, 2020

noblepaul added 2 commits August 10, 2020 18:49

merging with origin/jira/solr14680

870ba5f

merging with origin/jira/solr14680

30a4ec2

noblepaul changed the title ~~SOLR-14680: Provide simple interfaces to our concrete SolrCloud classes~~ SOLR-14680: Provide simple interfaces to our concrete SolrCloud classes (only API) Aug 10, 2020

noblepaul changed the title ~~SOLR-14680: Provide simple interfaces to our concrete SolrCloud classes (only API)~~ SOLR-14680: Provide simple interfaces to our cloud classes (only API) Aug 10, 2020

noblepaul added 2 commits August 10, 2020 21:32

reverting unnecessary changes

931eaa3

use enum for prefix

e21e9ea

noblepaul added 4 commits August 11, 2020 10:23

refactor

d2e8e12

isLeader() added

8c3bc25

baseUrl() added

0c2da56

url()

adebaf8

noblepaul merged commit 15ae014 into apache:master Aug 11, 2020

noblepaul added a commit that referenced this pull request Aug 13, 2020

SOLR-14680: Provide simple interfaces to our cloud classes (only API) (…

fc76180

…#1694)

gus-asf pushed a commit to gus-asf/lucene-solr that referenced this pull request Sep 4, 2020

SOLR-14680: Provide simple interfaces to our cloud classes (only API) (…

798f92b

…apache#1694)


		String baseUrl(boolean isV2);

		SimpleMap<ShardReplica> cores();


		import java.util.LinkedHashMap;

		public class LinkedSimpleHashMap<T> extends LinkedHashMap<CharSequence, T> implements SimpleMap<T> {


		import org.apache.solr.common.SolrException;

		public interface Config {

SOLR-14680: Provide simple interfaces to our cloud classes (only API) #1694

SOLR-14680: Provide simple interfaces to our cloud classes (only API) #1694

Conversation

noblepaul commented Jul 26, 2020 • edited

murblanc commented Jul 26, 2020

murblanc commented Jul 26, 2020

gus-asf commented Jul 26, 2020 • edited

gus-asf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noblepaul Jul 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murblanc commented Jul 26, 2020 • edited

chatman commented Jul 26, 2020

gus-asf commented Jul 27, 2020 • edited

noblepaul commented Jul 27, 2020 • edited

noblepaul commented Jul 27, 2020

noblepaul commented Jul 27, 2020 • edited

gus-asf commented Jul 27, 2020

murblanc commented Jul 27, 2020

murblanc commented Jul 27, 2020

chatman commented Jul 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murblanc commented Jul 30, 2020

murblanc commented Jul 30, 2020

noblepaul commented Jul 31, 2020

noblepaul commented Aug 10, 2020

noblepaul commented Aug 10, 2020

noblepaul commented Jul 26, 2020 •

edited

gus-asf commented Jul 26, 2020 •

edited

noblepaul Jul 27, 2020 •

edited

murblanc commented Jul 26, 2020 •

edited

gus-asf commented Jul 27, 2020 •

edited

noblepaul commented Jul 27, 2020 •

edited

noblepaul commented Jul 27, 2020 •

edited