Skip to content

Improved overflow support: Indexing rescanner#26

Merged
sungshik merged 18 commits intoimproved-overflow-support-mainfrom
improved-overflow-support/indexing-rescanner
Mar 11, 2025
Merged

Improved overflow support: Indexing rescanner#26
sungshik merged 18 commits intoimproved-overflow-support-mainfrom
improved-overflow-support/indexing-rescanner

Conversation

@sungshik
Copy link
Contributor

@sungshik sungshik commented Mar 5, 2025 •
edited
Loading

This PR implements the generation of synthetic CREATED/MODIFIED/DELETED events for DIRTY files to auto-handle overflow events. There are two new classes: IndexingRescanner and BaseFileVisitor. To conveniently reuse some of the functionality of MemorylessRescanner, there are refactoring-kind-of changes in that class, too.

@sungshik sungshik mentioned this pull request Mar 7, 2025
Merged
7 tasks
Base automatically changed from improved-overflow-support/overflow-policies-for-file-watches to improved-overflow-support-main March 7, 2025 12:38
@codecov
Copy link

codecov bot commented Mar 7, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 76.82927% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.1%. Comparing base (2aa7db2) to head (493165d).
Report is 19 commits behind head on improved-overflow-support-main.

Files with missing lines Patch % Lines
...g/swat/watch/impl/overflows/IndexingRescanner.java 79.1% 6 Missing and 4 partials ⚠️
...ing/swat/watch/impl/overflows/BaseFileVisitor.java 58.8% 5 Missing and 2 partials ⚠️
...swat/watch/impl/overflows/MemorylessRescanner.java 87.5% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@                        Coverage Diff                         @@
##             improved-overflow-support-main     #26     +/-   ##
==================================================================
+ Coverage                              80.0%   80.1%   +0.1%     
- Complexity                              110     120     +10     
==================================================================
  Files                                    14      16      +2     
  Lines                                   486     539     +53     
  Branches                                 47      53      +6     
==================================================================
+ Hits                                    389     432     +43     
- Misses                                   69      75      +6     
- Partials                                 28      32      +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sungshik
sungshik commented Mar 7, 2025
View reviewed changes
Copy link
Contributor Author

@sungshik sungshik left a comment

Choose a reason for hiding this comment

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

Clarifying comments

@sungshik sungshik marked this pull request as ready for review March 7, 2025 17:08
@sungshik sungshik requested a review from DavyLandman March 7, 2025 17:08
@sungshik sungshik mentioned this pull request Mar 10, 2025
Closed
DavyLandman
DavyLandman approved these changes Mar 10, 2025
View reviewed changes
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

On first glance it looks good. I'm curious about the perf impacts of this.

I'm curious what the effect is of replacing the existing one this one?

As in, this code is not yet run in the torture test.

sungshik
sungshik commented Mar 10, 2025
View reviewed changes
Copy link
Contributor Author

@sungshik sungshik left a comment

Choose a reason for hiding this comment

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

Thanks for the review! The low-hanging fruit is done. I'm leaving one conversation open and will first experiment a bit with performance.

@sungshik sungshik force-pushed the improved-overflow-support/indexing-rescanner branch from 79bc0b0 to 924492d Compare March 11, 2025 11:25
@sungshik
Copy link
Contributor Author

sungshik commented Mar 11, 2025

Regarding performance...

There happen to be two torture tests that use a non-recursive directory watch, so I ran them with/without indexing (repeated 20x). Here are the preliminary/anecdotal results, just to get a rough idea:

A working hypothesis to explain the difference is that Test 1 has 2000 threads that concurrently try to access the file system to index the test directory, while Test 2 has only 4 such threads.

In the spirt of "many small PRs", I'll go ahead and merge this one, and I'll add a TODO to #20 to separately conduct a more serious comparison (see also this comment) to understand the performance tradeoff, and optimize if needed/possible.

@sungshik sungshik merged commit d569c50 into improved-overflow-support-main Mar 11, 2025
13 checks passed
@sungshik sungshik deleted the improved-overflow-support/indexing-rescanner branch March 11, 2025 12:53
@sungshik sungshik mentioned this pull request Apr 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DavyLandman DavyLandman DavyLandman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@sungshik @DavyLandman