An aside: we should probably set up review board for this project. No need to make it necessary, but it might make it easier to review and thus improve the number of reviews.
I filed an issue a while back: https://issues.apache.org/jira/browse/INFRA-7630
there are a bunch of strings like coreNodeName, replica names, baseUrls whose relations I'm not sure about (I usually end up logging them and doing some ad-hoc comparisons). Perhaps turning these into types with proper conversions would make things clearer? Not necessary for this patch, just wondering your thoughts.
Sounds interesting to me - I agree, let's do it another issue though.
can we reduce the amount of duplicate code in these functions?
I'll look at it - I made an attempt at one point to converge some code and some small annoyance had me just revert it then.
Seconds seem limiting, why not milliseconds?
I think given the speed at which state updates propagate through the system, seconds is all the granularity that is needed, but I don't mind making it milliseconds.
Do we need this?
No, the listeners were for a failed path. I had thought I had taken them out in git, but probably used SmartGit to edit it in the index and not the working tree and lost the changes. Happens sometimes when I decide to edit with SmartGit.
I think the real issue is ZkStateReader is too complicated
Yeah, it kind of became a catch all class for anything above SolrZkClient and below ZkController. I'd just file a JIRA issue to at least start collecting ideas on some refactoring. ZkController could probably also use some editing.
Why can't we call createControlJetty?
I'll have to look. At one point, because it needed to be different, but I don't remember offhand if that is still the case.
There was some comment that this is only necessary temporarily. Is that already addressed?
That's https://issues.apache.org/jira/browse/SOLR-5473. I merged this up to that the first time it was committed. Once that was temporarily reverted and I updated to trunk, I commented that bit out. I'll leave it in until that issue is resolved without putting a ZkStateReader in ClusterState.