Bugzilla – Full Text Bug Listing
| Summary: | Want best-effort filesystem listening | ||
|---|---|---|---|
| Product: | [ECD] Orion | Reporter: | Evan Hughes <evan_hughes> |
| Component: | Server | Assignee: | Project Inbox <orion.server-inbox> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | enhancement | ||
| Priority: | P3 | CC: | ahunter.eclipse, john.arthorne |
| Version: | unspecified | ||
| Target Milestone: | 7.0 | ||
| Hardware: | PC | ||
| OS: | Linux | ||
| Whiteboard: | |||
|
Description
Evan Hughes
I've pushed the change to https://github.com/ehues/orion.server. I'm trying to push it to gerrit. Here is the Gerrit change: https://git.eclipse.org/r/#/c/28509/ Hi Evan, I looked at your changes and they look good, but I have a few comments. I initially had a bit of a problem "wrapping" my head around your wrapper pattern used in the FileStoreNotificationWrapper. While the wrapper pattern you are using for IFileStore works fine, I am wondering if you can take advantage of IAdaptable instead. It is used all over the place in Eclipse while your wrapper pattern would be "new". IFileStore and IFileSystem are already IAdaptable so I think this would be a good fit. We should also change the name of IFilesystemModificationListener to IFileStoreModificationListener, since we are tracking modifications to an IFileStore. We do not use IFileSystem in Orion so likely should not introduce the term Filesystem. (i.e. Filesystem should be FileStore). I would suggest that ChangeEvent be moved out into it's own class, renamed to FileStoreChangeEvent and have it extend EventObject. I also think we could add a timeStamp field to FileStoreChangeEvent. We also may want to distinguish between FileStoreChangeEvent.WRITE and CREATE versus UPDATE? Once we turn this on, the first adopter should be the Indexer. Currently we reindex based on a Job timer, we can change this to only reindex changed files based on IFileSystem change event notifications. Aww. I was hoping to see Gerrit in action. ;) (In reply to Anthony Hunter from comment #3) > While the wrapper pattern you are using for IFileStore works fine, I am > wondering if you can take advantage of IAdaptable instead. It is used all > over the place in Eclipse while your wrapper pattern would be "new". > IFileStore and IFileSystem are already IAdaptable so I think this would be a > good fit. I'm not sure what that would look like. What adaptation should I participate in? Are you suggesting that instead of explicitly calling 'wrap()' I should call getAdapter(FileStoreNotificationWrapper.class) or something similar? > We should also change the name of IFilesystemModificationListener to > IFileStoreModificationListener, since we are tracking modifications to an > IFileStore. We do not use IFileSystem in Orion so likely should not > introduce the term Filesystem. (i.e. Filesystem should be FileStore). Good point. > I would suggest that ChangeEvent be moved out into it's own class, renamed > to FileStoreChangeEvent and have it extend EventObject. Alright. I guess the source would be the servlet that is requesting the object. > I also think we could add a timeStamp field to FileStoreChangeEvent. That would be the file/folder timestamp reported by the filesystem after the operation has completed? In SCM we've found that repeated writes can fall into the granularity of filesystem timestamp, so they are less useful than one would hope. ymmv. What's the use case? > We also may want to distinguish between FileStoreChangeEvent.WRITE and > CREATE versus UPDATE? What's the use case? Do you want to do something beyond reading the flags passed into openOutputStream()? I can't figure out how to push my new commit to Gerrit. (In reply to Evan Hughes from comment #5) > I can't figure out how to push my new commit to Gerrit. You can't do it from Orion at the moment. If you're using EGit we can get it set up. PW (In reply to Paul Webster from comment #6) > You can't do it from Orion at the moment. If you're using EGit we can get > it set up. Thanks, I'll come by. (In reply to Evan Hughes from comment #4) > > I also think we could add a timeStamp field to FileStoreChangeEvent. > > That would be the file/folder timestamp reported by the filesystem after > the operation has completed? In SCM we've found that repeated writes can > fall into the granularity of filesystem timestamp, so they are less useful > than one would hope. ymmv. > > What's the use case? Your suggestion to use the filesystem timestamp is sufficient. > > > We also may want to distinguish between FileStoreChangeEvent.WRITE and > > CREATE versus UPDATE? > > What's the use case? > > Do you want to do something beyond reading the flags passed into > openOutputStream()? Ok, I see you only notify of a write based on this action, so we are good. (In reply to Evan Hughes from comment #4) > Aww. I was hoping to see Gerrit in action. ;) > > (In reply to Anthony Hunter from comment #3) > > While the wrapper pattern you are using for IFileStore works fine, I am > > wondering if you can take advantage of IAdaptable instead. It is used all > > over the place in Eclipse while your wrapper pattern would be "new". > > IFileStore and IFileSystem are already IAdaptable so I think this would be a > > good fit. > > I'm not sure what that would look like. What adaptation should I > participate in? Are you suggesting that instead of explicitly calling > 'wrap()' I should call getAdapter(FileStoreNotificationWrapper.class) or > something similar? I do not want to hold up the work so I will write up a what I had in mind when I get a chance. I will push your latest changes in gerrit, they look ok. Thanks Anthony. |