SOLR-18330: RTG requests to TLOG can short-circuit - #4697
Conversation
Optimization: RTG requests to a TLOG leader replica shouldn't use an extra local HTTP hop.
| replica -> | ||
| !builder.onlyNrt | ||
| || (replica.getType() == Replica.Type.NRT | ||
| || (replica.getType() == Replica.Type.TLOG |
There was a problem hiding this comment.
the TLOG check was needless and isn't friendly to custom/evolving replica types, even though admittedly unlikely.
There was a problem hiding this comment.
This change looks good to me too. Would be cool to have a isRealTime() utility.
There was a problem hiding this comment.
I agree it'd be nice to have a utility
| private boolean onlyNrt; | ||
| private boolean onlyRtg; |
There was a problem hiding this comment.
onlyNrt is misleading.
There was a problem hiding this comment.
IMO tracing is a nice way to show the byproduct of this optimization. Without the optimization, there wold be another embedded span for another HTTP request
HoustonPutman
left a comment
HoustonPutman
left a comment
There was a problem hiding this comment.
So I like the two changes regarding checking the near-real-time-ness of replicas.
Other than that, I don't really agree with the change from onlyNrt to onlyRtg. Maybe a different name would be better, since I understand why onlyNrt would be confusing for non-NRT leaders.
But RTG (I know not through the handler, just through the component) can be used with non-leader TLOG replicas, since a realTimeSearcher is not always needed.
| && (!onlyNrtReplicas || cloudDescriptor.getReplicaType() == Replica.Type.NRT)) { | ||
| && (!onlyRtgReplicas | ||
| || cloudDescriptor.getReplicaType() == Replica.Type.NRT | ||
| || cloudDescriptor.isLeader())) { |
There was a problem hiding this comment.
This change looks good to me. Though maybe we should have a utility method? Maybe if its gets used somewhere else. (Since this is a CloudDescriptor and the CloudReplicaSource change is using a Replica object, it can't be shared I guess)
| replica -> | ||
| !builder.onlyNrt | ||
| || (replica.getType() == Replica.Type.NRT | ||
| || (replica.getType() == Replica.Type.TLOG |
There was a problem hiding this comment.
This change looks good to me too. Would be cool to have a isRealTime() utility.
RTG always requests this: I'm not married to the NRT -> RTG swap but feel it's an improvement. |
2 participants
Optimization: RTG requests to a TLOG leader replica shouldn't use an extra local HTTP hop.
https://issues.apache.org/jira/browse/SOLR-18330