@LongRunningScript, search logging, warning providers improvements#7482
Open
labkey-adam wants to merge 8 commits intodevelopfrom
Open
@LongRunningScript, search logging, warning providers improvements#7482labkey-adam wants to merge 8 commits intodevelopfrom
labkey-adam wants to merge 8 commits intodevelopfrom
Conversation
labkey-jeckels
approved these changes
Mar 13, 2026
Contributor
labkey-jeckels
left a comment
labkey-jeckels
left a comment
There was a problem hiding this comment.
A couple of minor synchronization suggestions.
| STATIC_ADMIN_WARNINGS = messages; | ||
| synchronized (STATIC_WARNING_LOCK) | ||
| { | ||
| LazyInitializer.init(); // Invoke no-op method to ensure static initializer is executed |
Contributor
There was a problem hiding this comment.
To avoid double-calculation of warnings, this should really do another null check inside the synchronized block.
| public void register(WarningProvider provider) | ||
| { | ||
| _providers.add(provider); | ||
| STATIC_ADMIN_WARNINGS = null; |
Contributor
There was a problem hiding this comment.
Claude Code recommends setting this inside a synchronized(STATIC_WARNING_LOCK) due to a possible race condition where a recalculation is already in progress with the original list of providers.
| LazyInitializer.init(); // Invoke no-op method to ensure static initializer is executed | ||
| Warnings warnings = Warnings.of(new LinkedList<>()); | ||
| WarningService.get().forEachProvider(p -> p.addStaticWarnings(warnings, WarningService.get().showAllWarnings())); | ||
| STATIC_ADMIN_WARNINGS = Collections.unmodifiableList(warnings.getMessages());; |
Contributor
There was a problem hiding this comment.
Double semicolon.
Suggested change
| STATIC_ADMIN_WARNINGS = Collections.unmodifiableList(warnings.getMessages());; | |
| STATIC_ADMIN_WARNINGS = Collections.unmodifiableList(warnings.getMessages()); |
| """ | ||
| This script could take a long time to execute because it is {}. | ||
| Do NOT shut down or restart the server until this script and the rest of the upgrade is complete. | ||
| Any interruption will likely corrupt the schemas, requiring a database restore and a restart of the upgrade process.""", |
Contributor
There was a problem hiding this comment.
Claude Code recommends putting the closing """ on its own line to reduce confusion about indentation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tasks
Rationale
Several minor improvements to make admins' (and our) lives easier:
@LongRunningScriptSQL upgrade script "annotation" that indicates the script may take a long time to execute. A logged message gives the reason provided in the annotation and strongly discourages admins from shutting down the server mid-upgrade. (Inspired by a recent encounter with an impatient admin who kept shutting down their server during the INT to BIGINT migration.) Update that BIGINT script with the annotation.SqlScriptManagerhasBeenRun()andupdate()methods. These are pointless since SQL scripts are never re-run or updated in the database.LastIndexedfor all document providers. We already log a reason fordeleteIndex(), but I've seen cases where the server clears LastIndexed without it being obvious (to me) why.WarningProvideris registered. This allows providers that get registered after the first request the ability to still provide static warnings. Update theMailHelperwarning provider to take advantage of this.