SOLR-18394: Simplify JettyConfig construction - #4786
Conversation
The private constructor now reads its fields directly from the Builder, and the copy factory returns a clone of the builder snapshot kept by the built config. This removes the 10-argument constructor and the manual field-by-field copy logic, so adding a new Jetty option only requires a field and setter in the Builder plus the public final field.
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
I like the approach, mostly! I'm looking forward to this being done where it's most needed -- NodeConfig.
RE clone: why? Can we instead stop/prevent re-use, by adding a boolean like _used to the builder that then prevents build() from being called on it a second time? The cause of my concern is that the clone you implemented is shallow, which means additional build() has gotchas that I could easily see bugs stemming from. Or we could just document that limitation/issue.
…fiable Addresses review feedback: the shallow clone left extraServlets/extraFilters aliased between the builder, built configs, and copies from builder(JettyConfig), so a second build() or a mutated copy could silently change an existing config. The clone now copies both maps, and the built config exposes unmodifiable views.
…-jettyconfig # Conflicts: # solr/test-framework/src/java/org/apache/solr/embedded/JettyConfig.java
The clone is what lets You're right about the shallow maps though (an aliasing that predates this PR). |
2 participants
https://issues.apache.org/jira/browse/SOLR-18394
Test-framework-only refactor of
JettyConfig, prompted by feedback in #4738 that the10-arg private constructor had gotten out of control.
Builderand reads fields directly from itbuild()passes a deep clone, so builder reuse can't leak into a built configbuilder(JettyConfig other)clones the config's builder snapshot — manual copyfactory removed
call-site impact
Adding a new option now only touches the builder field/setter and the public field.