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/05/28 22:08:25 UTC
[GitHub] [lucene-solr] murblanc opened a new pull request #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state
murblanc opened a new pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542
# Description
Session used for replica placement computation was wrongly built to only account for state already in Zookeeper, therefore ignoring all in progress past Collection API operations. The Session returned to the cache was also the wrong one not including the changes to the cluster state resulting from the placement computation.
# Solution
The Session used for computation is now cloned from the cached session without trying to validate state in Zookeeper where that state might not yet be.
# Tests
Ran solr tests.
# Checklist
Please review the following and check all that apply:
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [X] I have developed this patch against the `master` branch.
- [X] I have run `ant precommit` and the appropriate test suite.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
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 merged pull request #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state
Posted by GitBox <gi...@apache.org>.
noblepaul merged pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542
----------------------------------------------------------------
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 #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state
Posted by GitBox <gi...@apache.org>.
noblepaul commented on pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542#issuecomment-641785613
@murblanc
I've fixed a few things pls review & I can merge this
----------------------------------------------------------------
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 #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state
Posted by GitBox <gi...@apache.org>.
murblanc commented on pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542#issuecomment-641927858
> @murblanc
> I've fixed a few things pls review & I can merge this
Added one comment. Looks good, thanks for picking this one up.
----------------------------------------------------------------
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 #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state
Posted by GitBox <gi...@apache.org>.
murblanc commented on a change in pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542#discussion_r438038559
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java
##########
@@ -644,15 +643,13 @@ public Session cloneToNewSession(SolrCloudManager cloudManager) {
List<Clause> expandedClauses = new ArrayList<>(this.expandedClauses);
List<Row> matrix = new ArrayList<>(nodes.size());
+ Map<String, Row> copyNodes = new HashMap<>();
+ for (Row oldRow: this.matrix) {
Review comment:
Tradeoff with this one time matrix walk and copy is that we copy all nodes, even those that might not be needed (because not present in parameter `nodes`). I do like the approach though it's cleaner than the code I wrote.
Maybe keep this initial loop but just store Row references, then do the copy below when newRow is needed? (there's already an if bloc when it's null, so copy action can easily be placed in else bloc).
----------------------------------------------------------------
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 #1542: SOLR-14347: fix cached session update to not depend on Zookeeper state
Posted by GitBox <gi...@apache.org>.
noblepaul commented on a change in pull request #1542:
URL: https://github.com/apache/lucene-solr/pull/1542#discussion_r438043059
##########
File path: solr/solrj/src/java/org/apache/solr/client/solrj/cloud/autoscaling/Policy.java
##########
@@ -644,15 +643,13 @@ public Session cloneToNewSession(SolrCloudManager cloudManager) {
List<Clause> expandedClauses = new ArrayList<>(this.expandedClauses);
List<Row> matrix = new ArrayList<>(nodes.size());
+ Map<String, Row> copyNodes = new HashMap<>();
+ for (Row oldRow: this.matrix) {
Review comment:
>even those that might not be needed
I believe it's a reasonable tradeoff because most of the times nodes do not go away. Yes, we make an extra copy in those rare occasions, but it should be 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