Skip to content

Improved overflow support: Indexing rescanner - #26

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

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

Conversation

@sungshik

@sungshiksungshik commented Mar 5, 2025

Copy link
Copy Markdown
Contributor

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.

@sungshiksungshik mentioned this pull request Mar 7, 2025
7 tasks
Base automatically changed from improved-overflow-support/overflow-policies-for-file-watches to improved-overflow-support-mainMarch 7, 2025 12:38
@codecov

codecovBot commented Mar 7, 2025

Copy link
Copy Markdown

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 linesPatch %Lines
...g/swat/watch/impl/overflows/IndexingRescanner.java79.1%6 Missing and 4 partials ⚠️
...ing/swat/watch/impl/overflows/BaseFileVisitor.java58.8%5 Missing and 2 partials ⚠️
...swat/watch/impl/overflows/MemorylessRescanner.java87.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.

@sungshiksungshik left a comment

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.

Clarifying comments

@DavyLandmanDavyLandman left a comment

Copy link
Copy Markdown
Member

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.

Comment threadsrc/main/java/engineering/swat/watch/impl/overflows/IndexingRescanner.java Outdated

@sungshiksungshik left a comment

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.

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
sungshikforce-pushed the improved-overflow-support/indexing-rescanner branch from 79bc0b0 to 924492dCompareMarch 11, 2025 11:25
@sungshik

Copy link
Copy Markdown
ContributorAuthor

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-mainMar 11, 2025
@sungshik
sungshik deleted the improved-overflow-support/indexing-rescanner branch March 11, 2025 12:53
@sungshiksungshik mentioned this pull request Apr 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@sungshik@DavyLandman