[GitHub] [lucene-solr] noblepaul commented on pull request #1684: SOLR-14613: strongly typed placement plugin interface and implementation
noblepaul commented on pull request #1684: URL: https://github.com/apache/lucene-solr/pull/1684#issuecomment-686839493 I see a consistent theme here. We are always combing through comments with a lens to see if the tone is correct, if somebody uses the correct case in typing or if the feedback is warm and fuzzy and caring. While it's a good in general, we miss the whole point of a review and feedback. The purpose is to ensure that the feature/change is - correct - performant/efficient - user-friendly The person who is doing a review is already doing a great favour by spending time to do a review. Let's be aware that the PR owner is probably paid to do that work and the reviewer is not. So, I have deep respect for anyone who reviews my PRs and gives suggestions (@dsmiley ) you are one of the devs who review a lot of my PRs and I'm grateful for what you do and you have the nicest demeanour while doing it. There are others like Uwe or Muir, who occasionally gives me a feedback without being very kind. I'm grateful to them because they have spent time to go through my PR using their personal time. They also bring a wealth of expertise that I probably don't have. The fact is, all of has the same objective: to make the product better. We are all not paid by the same employer or some of us may not even be paid at all. Let's focus more on the content and less on the form. cheers. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on pull request #1684: SOLR-14613: strongly typed placement plugin interface and implementation
noblepaul commented on pull request #1684: URL: https://github.com/apache/lucene-solr/pull/1684#issuecomment-686359748 > By all means, if you disagree so strongly with the approach presented here then please do so I clearly have objections to the verbosity of the proposals here. This does not mean that what I propose will be perfect. Nobody can design "the perfect system". A perfect system does not exist. I'm trying to give suggestions so that we can eliminate those obvious problems. At this point, two of you are invested in this feature and I totally understand your compulsions. > The impl. can be barebones, but the API that we define is going to stay with us for the next couple years at least, so it pays off to think seriously how to do it in an elegant and efficient way. What I see is a largescale undertaking to rewrite everything that we have in Solr . How else will you justify the presence of a class such as this https://github.com/apache/lucene-solr/blob/e385e7083bd4e0263e563d3cf0cb53b0e03e/solr/core/src/java/org/apache/solr/cluster/placement/Replica.java in a framework that is designed to just find out nodes to place replicas in a cluster? We do not want to support such a massive framework . I strongly recommend that we do not make duplicates for what we have today just for this particular framework. We already have an `AssignStrategy` interface. AFAIK, the only place where we have problem is in the `NodeStateProvider` . Let's add a method `SolrCloudManager#getNodepropertyFetcher()` and a `NodePropertyFetcher` interface and start reusing the `AssignStrategy` interface itself. That way we do not need any new framework. Most likely there won't be too many implementations of this framework. Just in case you wish to load this plugin from packages , My PR (#1684 ) has the necessary code to load this prom package. (That's optional at this point) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on pull request #1684: SOLR-14613: strongly typed placement plugin interface and implementation
noblepaul commented on pull request #1684:
URL: https://github.com/apache/lucene-solr/pull/1684#issuecomment-686327240
> Here's an example @noblepaul of how your proposal would be used (and if I
misunderstood something, please correct). nodes are the cluster nodes of
interest:
I'm not writing full PRs here.The objective was to give you hints as to how
to do things differently .
With a little more imagination, the code would look as follows.
The following could
- make requests in multiple thread
- minimal code
```
Iterable values = someFactory.createPropertyFetcher()
.withSystemProperty("AvailabilityZone")
.withCoresCount()
.fetchFromAllNodes()
```
or
```
PropertyValues values = someFactory.createPropertyFetcher()
.withSystemProperty("AvailabilityZone")
.withCoresCount()
.fetchFromNode("nodeName")
```
The common theme I see here is instead of trying to understand the spirit of
suggestions, you are trying to pick holes in the suggestions. I could as well
create a full framework and give a PR. Then you would say that "I do not want
to work on somebody else's design".
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on pull request #1684: SOLR-14613: strongly typed placement plugin interface and implementation
noblepaul commented on pull request #1684: URL: https://github.com/apache/lucene-solr/pull/1684#issuecomment-686321426 The consistent theme I see here is A. Here's is my elegant solution to the problem B. I think the solution is This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
[GitHub] [lucene-solr] noblepaul commented on pull request #1684: SOLR-14613: strongly typed placement plugin interface and implementation
noblepaul commented on pull request #1684: URL: https://github.com/apache/lucene-solr/pull/1684#issuecomment-686318299 > don't like an explosion of classes but I think this is highly mitigated by them being defined as simple inner classes of one outer class, as Ilan has done. Yeah, the solution to API surface area problem is to make them inner classes/interfaces. This is such a elegant & simple proposition. We should try the same in other places in Solr where there is a surface area problem. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
