Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update with latest listen version #333

Closed
wants to merge 6 commits into from
Closed

Conversation

marawan31
Copy link
Contributor

No description provided.

@marawan31 marawan31 closed this Aug 3, 2015
@e2
Copy link
Contributor

e2 commented Aug 3, 2015

I just wanted to suggest that the best way to optimize polling is to "intelligently" avoid directories which haven't changed. Listen has a Record class, but currently it does not store enough directory information to make this work (needs to be implemented).

If your use case is slow (for whatever reason) - open an issue and let me know.

@marawan31
Copy link
Contributor Author

Hi,

Thanks for your reply. Actually the work I am doing after I receive the
list of changed directories is slow, not actually retrieving the
directories. So I was fixing it by changing the part that calculates how
much time to wait after polling, since you consider the processing time.
Also my second issue was that the file I was checking for updates was never
considered modified. The reason is because this specific file is always
open in the program that modifies it so the date modified attribute never
gets updated and since that's what listen looks for in polling this file is
never considered modified. So the fix for this would be to simply use the
file size instead. It's not very clean but for my use case it works. I also
have a suggestion: when using jruby 'wdm' doesn't work since it uses a c
extension I am guessing so an alternative to avoid polling on windows would
be to use jruby-notify.

I just wanted to suggest that the best way to optimize polling is to
"intelligently" avoid directories which haven't changed. Listen has a
Record class, but currently it does not store enough directory information
to make this work (needs to be implemented).

If your use case is slow (for whatever reason) - open an issue and let me
know.


Reply to this email directly or view it on GitHub
#333 (comment).

@e2
Copy link
Contributor

e2 commented Aug 4, 2015

Actually the work I am doing after I receive the list of changed directories is slow, not actually retrieving the directories.

What I meant is that Polling does probably way too many unnecessary checks - most could be avoided if information in directory entries are checked. But that probably doesn't matter in your case...

Also my second issue was that the file I was checking for updates was never considered modified.

That's weird. I understand turning off Access time, but not updating the Modified time seems really strange.

If the file isn't too big, you could consider the md5sum check instead. Unless the file is huge, hashing with MD5 is actually quite fast - it's worth testing with debugging options on (to see the overhead).

As for wdm vs jruby-notify, Listen is more of a "platform independent wrapper" - so if you're using JRuby on Windows (which seems like a rare use case), you can probably just use jruby-notify directly.

Otherwise, I'd switch to using MRI on Windows if possible (unless you have a really good reason to stick with JRuby).

I'm a Linux guy, so I'd prefer not to "touch" anything regarding the Windows implementation, because I can't test those properly. But if you have any insights/issues (or useful PRs) - let me know.

@marawan31
Copy link
Contributor Author

Yea it really is strange that the modified time never gets changed. I can't
really understand how because i don't have access to the source code of the
program that is changing the file.
I didn't try with md5 since the file can be as big as 20MB which is kind of
big, also it is on an smb share so downloading the whole file every time to
do an md5 check is out of the question.
I have to use jruby for several reasons one of which is mssql access (since
jruby can use an sql server adapter).
jruby-notify lacks a lot of useful features that listen has.
I have made the modification that I sent you on the previous comment to test
it out and it's working great so far.

Thanks for your help

On Tue, 4 Aug 2015 at 15:17 Cezary Baginski notifications@github.com
wrote:

Actually the work I am doing after I receive the list of changed
directories is slow, not actually retrieving the directories.

What I meant is that Polling does probably way too many unnecessary checks

  • most could be avoid if information in directory entries are checked. But
    that probably doesn't matter in your case...

Also my second issue was that the file I was checking for updates was
never considered modified.

That's weird. I understand turning off Access time, but not updating the
Modified time seems really strange.

If the file isn't too big, you could consider the md5sum check instead.
Unless the file is huge, hashing with MD5 is actually quite fast - it's
worth testing with debugging options on (to see the overhead).

As for wdm vs jruby-notify, Listen is more of a "platform independent
wrapper" - so if you're using JRuby on Windows (which seems like a rare use
case), you can probably just use jruby-notify directly.

Otherwise, I'd switch to using MRI on Windows if possible (unless you have
a really good reason to stick with JRuby).

I'm a Linux guy, so I'd prefer not to "touch" anything regarding the
Windows implementation, because I can't test those properly. But if you
have any insights/issues (or useful PRs) - let me know.


Reply to this email directly or view it on GitHub
#333 (comment).

@e2
Copy link
Contributor

e2 commented Aug 4, 2015

Yeah, SMB makes the request for the file. Setups with network folders and/or VMs are tough to optimize.

I opened an issue here: #335

It's not critical, so I probably won't have time to implement it - but if you reduce/improve the PR, I could add the specs/tests and try and get this tested and released.

I'm sure you're busy with a project (and so am I), but if you do find time to make these changes and test them (so they are generic for everyone), I'm sure a lot of people will benefit (even on OSX or on FAT, where hashing is hit more often).

Suggestions for PR:

  • don't update the version number in a PR, since changing it is part of the gem releasing process
  • I wouldn't bother with an option for listen, since a size mismatch means the file was modified, period
  • the Record should store the file size (not just the mtime)

Anyway, I'm glad Listen is working for you.

Have a nice day.

@marawan31
Copy link
Contributor Author

The modifications aren't too long to implement so i decided to do them.
Hope this helps someone.

On Tue, 4 Aug 2015 at 17:34 Cezary Baginski notifications@github.com
wrote:

Yeah, SMB makes the request for the file. Setups with network folders
and/or VMs are tough to optimize.

I opened an issue here: #335 #335

It's not critical, so I probably won't have time to implement it - but if
you reduce/improve the PR, I could add the specs/tests and try and get this
tested and released.

I'm sure you're busy with a project (and so am I), but if you do find time
to make these changes and test them (so they are generic for everyone), I'm
sure a lot of people will benefit (even on OSX or on FAT, where hashing is
hit more often).

Suggestions for PR:

  • don't update the version number in a PR, since changing it is part
    of the gem releasing process
  • I wouldn't bother with an option for listen, since a size mismatch
    means the file was modified, period
  • the Record should store the file size (not just the mtime)

Anyway, I'm glad Listen is working for you.

Have a nice day.


Reply to this email directly or view it on GitHub
#333 (comment).

@e2
Copy link
Contributor

e2 commented Aug 5, 2015

Thanks - I'll probably have to refactor the directory entry comparing completely, since the file class is getting a bit too complex.

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.

None yet

2 participants