You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@solr.apache.org by Justin Sweeney <ju...@gmail.com> on 2022/12/30 22:36:44 UTC

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: Refactoring ZkStateReader

Posted by Noble Paul <no...@gmail.com>.

Thanks Justin

This is one of the beasts in Solr

On Sat, Dec 31, 2022, 9:37 AM Justin Sweeney <ju...@gmail.com>
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
>

Re: Refactoring ZkStateReader

Posted by David Smiley <ds...@apache.org>.

Makes sense -- thanks for tackling this!

~ David Smiley
Apache Lucene/Solr Search Developer
http://www.linkedin.com/in/davidwsmiley


On Fri, Dec 30, 2022 at 5:37 PM Justin Sweeney <ju...@gmail.com>
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
>