murblanc commented on pull request #1684:
URL: https://github.com/apache/lucene-solr/pull/1684#issuecomment-684063923
> I totally disagree that my proposal will be not performant. It has zero
impact on performance whatsoever.
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:
```
for (Node node : nodes) {
PropertyFetcher pf = someFactory.createPropertyFetcher();
pf.onNode(node).withSystemProperty("AvailabilityZone").withCoresCount();
PropertyValues pv = pf.fetchValues();
// Use the values pv.coreCount() and
pv.systemProperty("AvailabilityZone") if they got returned, store them
somewhere...
}
```
The calls to `fetchValues()` are sequential in the plugin code. The
underlying implementation **cannot** make them concurrent because it gets them
one by one and needs to answer first.
If a plugin wants to fetch in parallel (say a large cluster), there's
complexity involved in implementing parallel fetches: create threads, start
them, join then and collect results, dispose of the threads... and we likely
prefer not to have plugin start to create threads in uncontrolled ways.
That's why I say it's inefficient. It's not the fetching itself that would
identical to the proposal in the PR.
The proposal in the PR is the only way I've found to push the complexity of
multithreading or other to the implementation (that **we write**, not plugin
implementors) and make plugins dev easier and immediately efficient (or not
efficient, but then we can improve our implementation).
It also covers the case when plugin code doesn't know which node to send the
request to (for example once we add `withDocCount(collection, shard)` there's
nothing telling which node this should go to, yet a specific node has to be
picked to serve that data and I want this request to be "batchable" with other
requests for data to the node as above).
> The only issue I see with that is "it's not strongly typed". It's a matter
of aesthetics at this point.
Your proposal above **is** strongly typed so from that perspective I'm fine
with it. If you find a way with this proposal to **group the fetching of
everything from each node in one go** (the `withDocCount` issue that we don't
know where it should go), and **if there's a way for the framework - **not the
plugin writer** - to invest and make this efficient** I'm okay with it.
If really what you don't like is the number of PropertyValue interfaces (for
the record, they have just one method each) i'm willing to use only a single
per return type, or even a single one having getters returning Optional<type>
for all supported types (`HardwareType` for disk is one, unless we want to
return a string for it, then of course `Integer`, `Long`, `Double`, `String`).
This will save a few interfaces (3 exactly as of now, but then when we add new
types of keys the interface will likely already be available), and it will
simplify (to a lesser degree) the implementations because even though two
property values might return the same type, the value is not extracted in the
same way. If you look at `AbstractNodePropertyKey` in the PR, the constructor
is where the snitch type is matched, so if we have fewer implementation we'll
have to move that code to some other place.
----------------------------------------------------------------
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
