gmethvin / directory-watcher

A cross-platform Java recursive directory watcher, with a JNA macOS watcher and Scala better-files integration
Apache License 2.0
265 stars 34 forks source link

Option for disabling hashing #14

Closed chromy96 closed 6 years ago

chromy96 commented 6 years ago

Pull request contains option to define property in config file (using typesafe config) that will skip hashing of files. Configuration is false by default (defined in reference.conf).

chromy96 commented 6 years ago

Thanks for the comments @gmethvin. I will update accordingly. Regarding the config, I was hoping to avoid increasing the number of parameters of create method. That said, I agree with you that library should not introduce new dependencies if not necessary. I will redo the change so it uses the boolean parameter.

gmethvin commented 6 years ago

Yeah, if you have other ideas for the configuration I'm open to hearing them. A builder would be not so bad:

DirectoryWatcher.builder()
  .enableHashing(false)
  .paths(paths)
  .listener(evt => {
    System.out.println("Got event: " + evt);
  })
  .build()
  .watch();

I think for now we can just add the additional parameter.

chromy96 commented 6 years ago

I've pushed the change that addresses your comments.

jvican commented 6 years ago

I think that this option should be false by default. There are many applications that have idempotent functions (or functions checking these hashes and de-duplicating the evaluation). In the spirit of keeping backwards compatibility, I think it's important that we try to avoid these "insidious" performance bugs that can occur by accident.

jvican commented 6 years ago

Correcting myself: given that this option was already enabled by default and I've completely misunderstood the intent of this pull request, I withdraw my comment. I'm happy that there's work on this area -- I had no idea that directory-watcher was hashing the files by default. This is good news in my side since I may be able to gain some ms in compilation. :smile:

chromy96 commented 6 years ago

@gmethvin Last 2 commits address the your comments

gmethvin commented 6 years ago

@chromy96 This looks good besides the few things I mentioned in my last review. It'd be good to get this merged soon, so I'm happy to take over if you don't have the time. Thanks again for your help here.

chromy96 commented 6 years ago

Thanks, @gmethvin I can address the comments for the weekend, if that's not too late.

gmethvin commented 6 years ago

@chromy96 Yeah that's fine. I just didn't see any activity in a few weeks so I was just checking :)

chromy96 commented 6 years ago

Great. Yeah, was little busy finishing things before vacation :)

chromy96 commented 6 years ago

@gmethvin could you retrigger the AppVeyor build? It failed due to remote service unavailability.

gmethvin commented 6 years ago

Released as v0.6.0.

chromy96 commented 6 years ago

Excellent, thanks :)