Refactoring ZkStateReader
Hi all, ZkStateReader ( https://github.com/apache/solr/blob/main/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java) is a complicated class with thousands of lines and numerous inner classes. This makes approaching this class overly complex. I was recently investigating some changes involving this class and I think this class could use some refactoring to make changes easier to implement and review. This class is currently handling essentially all ZK data: cluster properties, collection properties, collection state, live nodes, etc. The handling of each of these could instead be delegated to other classes making it much more clear how to approach changing logic for a specific data type in ZK. This will also make it easier to review changes as well as deprecate unnecessary ZK elements in the future. I've taken a stab at a *draft* PR refactoring out the handling of Live Nodes specifically: https://github.com/apache/solr/pull/1258. This is still WIP and just here to provide an idea of what this could look like. I wanted to surface this onto the dev list to solicit feedback on if it makes sense to others to pursue refactoring here or if there are any reasons I'm missing for why this all needs to be located in a single class. Thanks and Happy New Year! Justin
Re: Solr 9.1.1 bugfix release
Still looking for a PR approval for the protobuf upgrade: https://github.com/apache/solr/pull/1252
Re: Refactoring ZkStateReader
Thanks Justin This is one of the beasts in Solr On Sat, Dec 31, 2022, 9:37 AM Justin Sweeney wrote: > Hi all, > > ZkStateReader ( > > https://github.com/apache/solr/blob/main/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ZkStateReader.java > ) > is a complicated class with thousands of lines and numerous inner classes. > This makes approaching this class overly complex. I was recently > investigating some changes involving this class and I think this class > could use some refactoring to make changes easier to implement and review. > > This class is currently handling essentially all ZK data: cluster > properties, collection properties, collection state, live nodes, etc. The > handling of each of these could instead be delegated to other classes > making it much more clear how to approach changing logic for a specific > data type in ZK. This will also make it easier to review changes as well as > deprecate unnecessary ZK elements in the future. > > I've taken a stab at a *draft* PR refactoring out the handling of Live > Nodes specifically: https://github.com/apache/solr/pull/1258. This is > still > WIP and just here to provide an idea of what this could look like. > > I wanted to surface this onto the dev list to solicit feedback on if it > makes sense to others to pursue refactoring here or if there are any > reasons I'm missing for why this all needs to be located in a single class. > > Thanks and Happy New Year! > > Justin >
