SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

SOLR-18149: remove ClusterState.createFromJson -- deprecated - #4777

Merged
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson
Aug 24, 2026
Merged

SOLR-18149: remove ClusterState.createFromJson -- deprecated#4777
dsmiley merged 5 commits into
apache:mainfrom
serhiy-bzhezytskyy:SOLR-18149-undeprecate-createfromjson

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Contributor

https://issues.apache.org/jira/browse/SOLR-18149

Un-deprecates ClusterState.createFromJson instead of removing it. The only reason it was deprecated (SOLR-17561) was "doesn't support legacy configName location" -- confirmed on the ticket that this is a Solr 8 concern, not relevant from 9 forward, and no such legacy-location code path exists anywhere in the current parsing. No behavior change; the caveat was already dead documentation.

3 tests, 0 failures.

AI-assisted (Claude Sonnet 5)

The only reason it was deprecated (SOLR-17561) was 'doesn't support
legacy configName location' -- David Smiley confirmed on the ticket
that this is a Solr 8 concern, not relevant from 9 forward, and no
such legacy-location code path exists anywhere in the current parsing.
No behavior change; the caveat was already dead documentation.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no; this should be removed not un-deprecated. Please offer an explanation as to why it should stay if you disagree.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh please add the no-changelog label

@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

@epugh label's on, but the changelog check still shows failed from before it was added -- could you re-trigger it?

…ing it
Per review feedback, this should be removed rather than un-deprecated -- the
legacy-configName caveat that justified un-deprecating it doesn't change the
fact that it's a thin wrapper: parse bytes to a Map, then call
createFromCollectionMap. Inlined that at all 7 call sites (6 test, 1
production in BackupManager) instead of keeping the wrapper alive.
Also removes CoreContainer.setWeakStringInterner() and
ClusterState.setStrInternerParser()/STR_INTERNER_OBJ_BUILDER: that
configuration only ever fed createFromJson's parsing step. The actual
production hot path (ZkStateReader.fetchCollectionState()) already parses
ZK data with plain Utils.fromJSON(), bypassing the interner entirely -- so
this configuration was already dead before this change, and removing
createFromJson makes that unreachable for certain.
Deleted the two ClusterStateTest assertions that fed createFromJson empty/null
bytes to check its defensive fallback: that behavior belonged to the removed
wrapper, not to any surviving public API, so there's nothing left to regression
test there.
@serhiy-bzhezytskyy

Copy link
Copy Markdown
ContributorAuthor

Removed createFromJson entirely instead of un-deprecating it -- inlined its parse-then-createFromCollectionMap body at all 7 call sites (6 test, 1 production).

Also removed CoreContainer.setWeakStringInterner()/ClusterState.setStrInternerParser(): that config only ever fed createFromJson's parsing, and the real production path (ZkStateReader.fetchCollectionState()) already bypasses it with plain Utils.fromJSON(). Was already dead, now provably unreachable.

@dsmileydsmiley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bigger over-arching change here isn't the removal of this static method. It's that you have identified that SOLR-16328 failed to do what it was supposed to do -- the string-interner thing Noble did. I recommend that you proceed here and completely clean up all vestigial aspects of it (not sure if anything is left TBH). See https://issues.apache.org/jira/browse/SOLR-16328?focusedCommentId=18107135&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-18107135

Comment threadsolr/core/src/java/org/apache/solr/core/backup/BackupManager.java Outdated
@dsmiley
dsmiley requested a review from magibneyAugust 23, 2026 15:29
…tate
Utils.fromJSON already returns Map.of() for a zero-length/null array --
the ternary duplicated that check instead of relying on it.
@serhiy-bzhezytskyy

serhiy-bzhezytskyy commented Aug 24, 2026

Copy link
Copy Markdown
ContributorAuthor

SOLR-16328 vestiges: already done -- grepped the whole tree for STR_INTERNER_OBJ_BUILDER, WeakStringInterner, setStrInternerParser, any config/doc reference: zero hits anywhere. My earlier commit (removing createFromJson + CoreContainer.setWeakStringInterner() + ClusterState.setStrInternerParser()) already covered it -- landed before your research comment, for the same reason you found: the interner never reached the real ZK hot path.

@epughepugh self-assigned this Aug 24, 2026
ClusterState c_state = ClusterState.createFromJson(-1, arr, Set.of(), Instant.EPOCH, null);
@SuppressWarnings("unchecked")
Map<String, Object> stateMap = (Map<String, Object>) Utils.fromJSON(arr, 0, arr.length);
ClusterState c_state =

@epughepughAug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, I guess it's fine to leave in a deprecation targeted PR but really? c_state???

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #4810.

@dsmileydsmiley changed the title SOLR-18149: un-deprecate ClusterState.createFromJsonSOLR-18149: remove ClusterState.createFromJson -- deprecatedAug 24, 2026
@dsmileydsmiley added this to the 10.x milestone Aug 24, 2026
@dsmiley
dsmiley merged commit a733331 into apache:mainAug 24, 2026
6 of 7 checks passed
@epugh

Copy link
Copy Markdown
Contributor

@dsmiley how are you handling the JIRA side? Are you resolving it when you do the backport to Solr 10?

@dsmiley

Copy link
Copy Markdown
Contributor

I'll close after backport.

dsmiley pushed a commit that referenced this pull request Aug 27, 2026
And removed mostly unused String intern on ClusterState. Prefer simplicity over dubious improvement we don't even have today.
(cherry picked from commit a733331)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@serhiy-bzhezytskyy@epugh@dsmiley