SOLR-15996: disabling PerReplicaState was resulting in DOWN replicas#644
SOLR-15996: disabling PerReplicaState was resulting in DOWN replicas#644
Conversation
| import java.util.Map; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.*; |
There was a problem hiding this comment.
please configure your editor to not do this
There was a problem hiding this comment.
This is becoming a huge problem. I'm new to IntelliJ, and it seems to be doing this to all my PRs too. When I used Eclipse, this wasn't possible. Can we somehow add this configuration to the "gradlew idea" task?
There was a problem hiding this comment.
I never run the idea task, IntelliJ seems to be able to import the gradle project natively for me. Occasionally I have to "restart and invalidate caches" to reimport it, but that's usually after I've been changing branches around a lot.
I couldn't find a checked in or shared IntelliJ settings file, although I feel like it used to exist?
Under the settings, Editor > Code Style > Java find the tab called imports and I have class count set to 99 and all of the packages to use import with * choices unchecked or deleted.
| @SuppressWarnings({"unchecked"}) | ||
| Map<String, Replica> replicas = Collections.EMPTY_MAP; |
There was a problem hiding this comment.
that was a merge issue
| coll.forEachReplica((s, replica) -> { | ||
| PerReplicaStates.State prsState = prs.states.get(replica.getName()); | ||
| if (prsState != null) { | ||
| if (prsState.state != replica.getState()) { |
There was a problem hiding this comment.
This if statement and the next one look like they are almost doing the same thing, is there a way to simplify this logic so that we are only getting/setting from modifiedSlices once?
There was a problem hiding this comment.
I thought about it. I thought this would make it simpler. suggestions are welcome
There was a problem hiding this comment.
Yea, I don't think I have any better ways.
| if(!enable) { | ||
| coll = updateReplicasFromPrs(coll, prs); | ||
| } |
There was a problem hiding this comment.
Is there still a race condition here? What happens if a replica goes down (or changes leadership) between the fetch and the persist which happens much later?
| } | ||
| } | ||
| } | ||
| class CommandWithClusterState extends ArrayList<ZkWriteCommand> { |
There was a problem hiding this comment.
This feels like bad design. We probably want to have composition over inheritance here, it feels really strange to be extending ArrayList just to give it an extra data field for what we're returning.
| } | ||
| } | ||
|
|
||
| private List<ZkWriteCommand> processMessage(ClusterState clusterState, |
There was a problem hiding this comment.
We might want to refactor this away from returning a list and instead return a ZkWriteCommand directly. Most of the operations are singleton commands so that is easy to handle. Then we could create a MultiCommand to handle the cases where that is not enough (looks like just down node, I think?). And the use case you're trying to address would be handled either by double-dispatch pattern or a similar cast to what we have in your PR now.
| coll.forEachReplica((s, replica) -> { | ||
| PerReplicaStates.State prsState = prs.states.get(replica.getName()); | ||
| if (prsState != null) { | ||
| if (prsState.state != replica.getState()) { |
There was a problem hiding this comment.
Yea, I don't think I have any better ways.
|
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution! |
|
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. |
3 participants
No description provided.