Skip to content

Conversation

@DavyLandman
Copy link
Member

@DavyLandman DavyLandman commented Sep 13, 2024

On-and-off I've been working on a nice reference implementation of a file changes watcher. Both the raw api, and the tests I could come up with.

The future will bring windows & linux (and maybe osx) specific implementation that can benefit from the native kernel api.

todo:

  • add license
  • setup deployment to somewhere like maven.
  • add code examples to the readme
  • review log levels

This better reflects other recursive implementations
@DavyLandman
Copy link
Member Author

@sungshik thanks for the thorough review, I think I've address all comments.

@DavyLandman
Copy link
Member Author

I'm still doubting the exact interface of the Watcher class though, curious on your thoughts.

Copy link
Contributor

@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.

I think the interface of the Watcher class is much clearer now; well done! My only suggestion is to rename the enum values a bit (but this is really a matter of taste, so just see how you feel about it). Longer term (not this PR), it could be nice to consider abstracting away also the usage of java.nio.file.Path in the public methods of Watcher and use, for instance, Rascal(-style) locations (if there's a use case for this, but I imagine there could be).

I also had a look at the other changes, and it all looks fine, except I think there are still races in BundledSubscription. I tried to come up with a fix, but it's tricky to do it without bigger synchronization blocks. Here's a proposal. The correctness guarantee I can give is: I'm not convinced it's wrong 😉. Also, with more effort, it might be possible to make the synchronized blocks a bit smaller (e.g., pull active.remove and active.add outside).

Copy link
Contributor

@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.

Had a look at the tests. Just a few small things 👍

void run(Path target) throws IOException;
}

private static void recursiveDelete(Path target) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a method deleteAllFiles in class TestDirectory

Copy link
Contributor

@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.

And a review of the non-test code since last review. Looks good!

@DavyLandman DavyLandman force-pushed the initial-implementation branch from 2b3ee52 to 982ce65 Compare November 11, 2024 10:27
@DavyLandman DavyLandman merged commit 378f29a into main Nov 11, 2024
10 checks passed
@DavyLandman DavyLandman deleted the initial-implementation branch December 10, 2024 13:58
Sign up for free to 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