Don't filter directories in watchman subscription

Review Request #4095 — Created Aug. 13, 2016 and submitted

stuhood
pants
3574, 3688
pants-reviews
kwlzn, patricklaw

As described on #3574, filtering changed directories meant that if a directory was deleted, we wouldn't get an event for the changed listing of its parent. It also wasn't really letting us avoid the underlying over-notification bug (linked in a comment here), because ignoring directories meant we needed to invalidate parent directories ourselves in FilesystemNode.

  • Drop type filter to include directory events.
  • Only invalidate the dirname of a file when it is contained in the root directory, since that is the only time when watchman will not send a directory event.
  • Fix test to not assume that a parent directory will be invalidated.

https://travis-ci.org/pantsbuild/pants/builds/155497553

  1. lgtm - handful of comments/questions.

  2. src/python/pants/engine/nodes.py (Diff revision 1)
     
     
     

    I don't believe we'd ever see . as the dirname due to the way the watchman subscriptions and events work - maybe you've seen otherwise tho?

  3. src/python/pants/engine/scheduler.py (Diff revision 1)
     
     

    this is probably still useful for end user debugging.

    1. Agreed. Will add this back and remove the messages about the individual nodes that were invalidated.

  4. src/python/pants/engine/scheduler.py (Diff revision 1)
     
     
     
     

    doesn't the existing boolean expression do exactly this just in a more concise form? i.e. it will short circuit and return a False before getting to the and? or is there another reason for this that I'm missing?

    1. Must have been a remnant of debugging. Reverted.

  5. it might be better to just add ['type', 'd'] here instead of turning off type filtering altogether?

    this would avoid events for all of the other types of non-files: https://facebook.github.io/watchman/docs/expr/type.html

    1. I think that if somebody dropped a named pipe or something else wacky in the repo, we'd want to blow up quickly for it, since our scandir will anyway.

    2. but this would blow up in the daemon/backend during an invalidation event where the user likely can't do anything about it. blowing up on the frontend side during scandir seems more actionable, no?

    3. No: it would cause the foreground to blow up... this would just invalidate our directory listing, and then the next foreground scandir would explode with the error, as expected.

      The alternative would be that our scandir would not be invalidated, and then much, much later when you restart pants, the cold run would explode.

    4. ah, I see what you mean.

  6. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Merged as d67cde7accf3c58e9eeb06f0be748d4c49da98fa

Loading...