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/09/01 01:20:13 UTC

[GitHub] [lucene-solr] noblepaul opened a new pull request #1813: SOLR-14613: No new APIs. use the existing APIs

noblepaul opened a new pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813


   WIP , do not commit. Untested
   
   We are spending too much time to create and manage a new set of APIs instead of solving the real problem at hand
   
   What does this PR do
   
   - No new interfaces. Uses the existing interface
   - Supports configuring `assign-startegy` in `clusterprops.json` . 
   - Supports 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] noblepaul commented on a change in pull request #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#discussion_r484682298



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -551,11 +555,29 @@ public AssignStrategyFactory(SolrCloudManager solrCloudManager) {
       this.solrCloudManager = solrCloudManager;
     }
 
+    @SuppressWarnings("unchecked")
     public AssignStrategy create(ClusterState clusterState, CloudConfig cloudConfig, DocCollection collection) throws IOException, InterruptedException {
       @SuppressWarnings({"unchecked", "rawtypes"})
       List<Map> ruleMaps = (List<Map>) collection.get("rule");
       @SuppressWarnings({"rawtypes"})
       List snitches = (List) collection.get(SNITCH);
+      Map<String, Object> props = solrCloudManager.getClusterStateProvider().getClusterProperties();
+      Map<String,Object> assignInfo = (Map<String, Object>) props.get("assign-strategy");

Review comment:
       We already have an endpoint called  `/api/cluster/plugin`  GET/POST
   
   Please refer to [SOLR-14404](https://issues.apache.org/jira/browse/SOLR-14404) for details
   
   it can look as follows
   
   ```
   curl -X POST -H 'Content-type:application/json' --data-binary '
   {
     "set-assign-strategy": {
     "class": "full.ClassName", 
     "key1" : "val1"
     }
   }' http://localhost:8983/api/cluster/plugins
   ```
   ```
   curl -X POST -H 'Content-type:application/json' --data-binary '
   {
     "set-assign-strategy":  null
   }' http://localhost:8983/api/cluster/plugins
   ```
   You may add your code [here](https://github.com/apache/lucene-solr/blob/f7cbde2ad85b656b7c7ac16acaafb05481fa75ba/solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java#L77)
   
   




----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#discussion_r484682298



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -551,11 +555,29 @@ public AssignStrategyFactory(SolrCloudManager solrCloudManager) {
       this.solrCloudManager = solrCloudManager;
     }
 
+    @SuppressWarnings("unchecked")
     public AssignStrategy create(ClusterState clusterState, CloudConfig cloudConfig, DocCollection collection) throws IOException, InterruptedException {
       @SuppressWarnings({"unchecked", "rawtypes"})
       List<Map> ruleMaps = (List<Map>) collection.get("rule");
       @SuppressWarnings({"rawtypes"})
       List snitches = (List) collection.get(SNITCH);
+      Map<String, Object> props = solrCloudManager.getClusterStateProvider().getClusterProperties();
+      Map<String,Object> assignInfo = (Map<String, Object>) props.get("assign-strategy");

Review comment:
       We already have an endpoint called  `/api/cluster/plugin`  GET/POST
   
   Please refer to [SOLR-14404](https://issues.apache.org/jira/browse/SOLR-14404) for details
   
   it can look as follows
   
   ```
   curl -X POST -H 'Content-type:application/json' --data-binary '
   {
     "set-assign-strategy": {
     "class": "full.ClassName", 
     "key1" : "val1"
     }
   }' http://localhost:8983/api/cluster/plugins
   ```
   
   You may add your code [here](https://github.com/apache/lucene-solr/blob/f7cbde2ad85b656b7c7ac16acaafb05481fa75ba/solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java#L77)
   
   




----------------------------------------------------------------
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 closed pull request #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul closed pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813


   


----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-686410621


   > Honestly nobody will do that. People would rather happily do the computation in client code
   
   Really? This is like saying that we only ever need collection admin APIs and we don't need any autoscaling. That may be the case for some people, but not for others. Also, not everybody would go and implement their own plugin, instead they would likely use someone else's plugin that is known to work for a particular scenario.
   
   I think the opposite of what you said is true - the expectation is that Solr will behave in a sane way and automatically do what's needed to "properly" use available resources, without too much manual handholding. The problem is of course that the definition of "properly" depends on the point of view and the scale of the problem - that's why we're having the discussion about plugins at all.
   
   Take Salesforce's case for example: they had to hack around the existing AssignStrategy to implement their own, because the default ones didn't work for them. In the process they had to learn and mess around with a lot of Solr internals. It would have been clearly better if they could just implement it as an installable plugin, using an abstract API that doesn't expose too many Solr internals.


----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-689885612


   This was created as a PoC. It has outlived its purpose, closing....


----------------------------------------------------------------
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] tflobbe commented on pull request #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
tflobbe commented on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-685124240


   > Supports configuring assign-strategy in clusterprops.json .
   
   @murblanc recently started [a thread](http://mail-archives.apache.org/mod_mbox/lucene-dev/202008.mbox/%3cCAGhHkK4MCrja3xS25FQOwMzmLQWNm-EN0PgLTvRR0ahkGqip7Q@mail.gmail.com%3e) in the dev list to discuss about configuration location. Can we try to resolve that first before we start adding configuration in different places? 


----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#discussion_r484682298



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -551,11 +555,29 @@ public AssignStrategyFactory(SolrCloudManager solrCloudManager) {
       this.solrCloudManager = solrCloudManager;
     }
 
+    @SuppressWarnings("unchecked")
     public AssignStrategy create(ClusterState clusterState, CloudConfig cloudConfig, DocCollection collection) throws IOException, InterruptedException {
       @SuppressWarnings({"unchecked", "rawtypes"})
       List<Map> ruleMaps = (List<Map>) collection.get("rule");
       @SuppressWarnings({"rawtypes"})
       List snitches = (List) collection.get(SNITCH);
+      Map<String, Object> props = solrCloudManager.getClusterStateProvider().getClusterProperties();
+      Map<String,Object> assignInfo = (Map<String, Object>) props.get("assign-strategy");

Review comment:
       We already have an endpoint called  `/api/cluster/plugin`  GET/POST
   
   Please refer to [SOLR-14404](https://issues.apache.org/jira/browse/SOLR-14404) for details
   
   it can look as follows
   
   ```
   curl -X POST -H 'Content-type:application/json' --data-binary '
   {
     "set-assign-strategy": {
     "class": "full.ClassName", 
     "key1" : "val1"
     }
   }' http://localhost:8983/api/cluster/plugins
   ```




----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
sigram commented on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-686349516


   This approach was something that I suggested for branch_8x only, and only because the Policy-based placements didn't work and I needed a critical fix. This approach has minimal impact on the existing API and configs so it still may be a good workaround to implement in 8x. However, for master/trunk we need a better design anyway.


----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
sigram edited a comment on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-686410621


   > Honestly nobody will do that. People would rather happily do the computation in client code
   
   Really? This is like saying that we only ever need collection admin APIs and we don't need any autoscaling. That may be the case for some people, but not for others. Also, not everybody would go and implement their own plugin, instead they would likely use someone else's plugin that is known to work for a particular scenario.
   
   I think the opposite of what you said is true - the expectation is that Solr will behave in a sane way and automatically do what's needed to "properly" use available resources, without too much manual handholding. The problem is of course that the definition of "properly" depends on the point of view and the scale of the problem - that's why we're having the discussion about plugins at all.
   
   Take Salesforce's case for example: they had to hack around the existing AssignStrategy to implement their own, because the default ones didn't work for them. In the process they had to learn and mess around with a lot of Solr internals. It would have been clearly better if they could just implement it as an installable plugin, using an abstract API that doesn't expose too many Solr internals.
   
   Edit: oh, and obviously one of the valid strategies would be to rely on provided placements (basically a do-nothing-strategy), but having this option as a plugin with a clean API is clearly better than just a bunch of messy hardcoded conditionals.


----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-686400974


   > However, for master/trunk we need a better design anyway.
   
   I would say that most users are actually happy if they do not have to implement a bunch of java interfaces, package a jar add it to Solr to just add a replica in the right place. Honestly nobody will do that. People would rather happily do the computation in client code and send that information as a input. If a DSL is available, they may use it. If not this will be of no interest to most users
   


----------------------------------------------------------------
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 #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#discussion_r484094742



##########
File path: solr/core/src/java/org/apache/solr/cloud/api/collections/Assign.java
##########
@@ -551,11 +555,29 @@ public AssignStrategyFactory(SolrCloudManager solrCloudManager) {
       this.solrCloudManager = solrCloudManager;
     }
 
+    @SuppressWarnings("unchecked")
     public AssignStrategy create(ClusterState clusterState, CloudConfig cloudConfig, DocCollection collection) throws IOException, InterruptedException {
       @SuppressWarnings({"unchecked", "rawtypes"})
       List<Map> ruleMaps = (List<Map>) collection.get("rule");
       @SuppressWarnings({"rawtypes"})
       List snitches = (List) collection.get(SNITCH);
+      Map<String, Object> props = solrCloudManager.getClusterStateProvider().getClusterProperties();
+      Map<String,Object> assignInfo = (Map<String, Object>) props.get("assign-strategy");

Review comment:
       Do you have code (and curl) examples of how the `assign-strategy` property or similar ones are stored in `clusterprops.json`?
   
   As I'm switching my PR to not use `solr.xml` but instead use `clusterprops.json`, I not only need to retrieve the plugin class name if any, but also associated configuration values whose names/keys and types are unknown since they are plugin specific.
   
   If I copy the code as written here, I assume an iteration over `assignInfo` would reveal all configured values, want to make sure this 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 pull request #1813: SOLR-14613: No new APIs. use the existing APIs

Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1813:
URL: https://github.com/apache/lucene-solr/pull/1813#issuecomment-687126415


   >Really? This is like saying that we only ever need collection admin APIs and we don't need any autoscaling. 
   
   I don't think I made myself clear.
   
   Users would definitely like Solr to place the replicas correctly. But if it means implementing a some plugin in java and packaging it in a jar & deploying it in their cluster, they would rather not do it.
   If it's as easy as writing down some DSL, they may use 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