Conversation
|
Lightbeam link |
allister-beamable
left a comment
There was a problem hiding this comment.
So, generally I think the changes are in a good direction, with one sort of architectural concern: it feels like we are adding moving parts to an already-complex system in hopes of making it more robust.
In the design tension of simplicity vs. featureful functionality, I think we are already on the "make it shiny and reactive" side of the bridge, so we may have to reserve true simplification for the moderate to far future.
All this to say: ship it! (but look for opportunities to simplify if possible)
| var entries = ContentService.ContentFileToLocalContentManifestEntries(contentFiles) | ||
| .ToArray(); |
There was a problem hiding this comment.
Is this the part that is slow when there are lots and lots of content entries?
| public static void HandleWatcherError(Exception exception, string pid, string manifestId, ILogger logger, | ||
| ChannelWriter<ChangedContentFile> changedContentFiles) | ||
| { | ||
| if (TryCreateWatcherRecoveryChange(exception, pid, manifestId, out var recoveryChange)) | ||
| { | ||
| logger.LogWarning( | ||
| "Content filesystem notifications were lost. Scheduling a full local content rescan. " + | ||
| "PID={Pid}, ManifestId={ManifestId}", | ||
| pid, | ||
| manifestId); | ||
| changedContentFiles.TryWrite(recoveryChange); | ||
| return; | ||
| } | ||
|
|
||
| logger.LogError( | ||
| exception, | ||
| "Content filesystem watcher failed. PID={Pid}, ManifestId={ManifestId}", | ||
| pid, | ||
| manifestId); | ||
| } |
There was a problem hiding this comment.
Something about this watcher error handler makes my brain itch, like it is a bridge too far in terms of moving parts. I see that we already have a filesystem watcher as part of the mechanism, but something bothers me about that.
Or, alternatively, could we make it clearer to the user that something is busted in a way that refreshing will fix, and give them a big prominent refresh button to fix it?
Third-alternatively, would it make sense to just have a heuristic where if you delete a bunch of junk the system triggers the full refresh when the volume of change exceeds a certain threshold (say 500 entries or 1000 entries)?
If these worries of mine spark thoughts for improvements, purpose of review achieved. If, on the other hand, you think this way is still the best way, go with it.
Fixes #4642
Brief Description
Large local content operations can overflow the filesystem watcher's internal buffer. When this happened, some file notifications were lost and the Unity Content window displayed an incomplete deleted count until the user manually refreshed it.
This change:
Verification
ContentWatcherRecoveryTests: 8/8 passed.Follow-up
Overflow recovery currently takes approximately 7–10 seconds in the large-content reproduction. That performance and observability work is intentionally outside the scope of this fix and is tracked separately in #4806 — Bulk local content changes can take 7–10 seconds to appear in the Unity Content window.
Checklist
Add those to list or remove the list below altogether:
Notes
When you are merging a feature branch into
main, please squash merge and make sure the final commit contains any relevent JIRA ticket number. If you are merging frommaintostaging, orstagingtoproduction, please use a regular merge commit.Does this introduce tech-debt? If so, have you added an entry to the Tech-debt document?