Issue #5086 Review and rework o.e.j.util.Scanner #5744
Conversation
Signed-off-by: Jan Bartel <[email protected]>
Signed-off-by: Jan Bartel <[email protected]>
Signed-off-by: Jan Bartel <[email protected]>
Signed-off-by: Jan Bartel <[email protected]>
| @@ -135,7 +136,7 @@ public WebAppScannerListener(AntWebAppContext awc) | |||
| } | |||
|
|
|||
| @Override | |||
| public void filesChanged(List<String> changedFileNames) | |||
| public void filesChanged(Set<String> changedFileNames) | |||
There was a problem hiding this comment.
Don't like this. File names are unique by definition, so there is no risk of duplicates. I would not add additional randomness caused by Set. If I really don't care, I'd use Collection -- the Set is supposed to be immutable anyway, right?
There was a problem hiding this comment.
Not sure I understand your point. If it's a List then by definition I can have duplicates in a List, but I can't in a Set. Plus, you can't rely on ordering as it depends on the filesystem in what order scanned directories are visited, so List is a furphy.
| @@ -140,7 +140,6 @@ protected void doStart() throws Exception | |||
| _scanner = new Scanner(); | |||
| _scanner.setScanDirs(files); | |||
| _scanner.setScanInterval(_scanInterval); | |||
| _scanner.setRecursive(_recursive); | |||
There was a problem hiding this comment.
Scanner.setRecursive() was deprecated, but there is no Javadoc about why.
This is the only usage of ScanningAppProvider._recursive which then should also be deprecated and removed?
| public boolean equals(Object obj) | ||
| { | ||
| return ((Event)obj)._filename.equals(_filename) && ((Event)obj)._notification == _notification; | ||
| } |
There was a problem hiding this comment.
Add correspondent hashCode(). Use Objects.hash(...) if possible.
| assertTrue(_queue.isEmpty()); | ||
|
|
||
| private final List<Listener> _listeners = new ArrayList<>(); | ||
| private final Map<String, TimeNSize> _prevScan = new HashMap<>(); | ||
| private final Map<String, TimeNSize> _currentScan = new HashMap<>(); | ||
| private AtomicInteger _scanCount = new AtomicInteger(0); |
| } | ||
|
|
||
| /** | ||
| * Scan all of the given paths. | ||
| */ | ||
| public void scanFiles() | ||
| public Map<String, MetaData> scanFiles() |
There was a problem hiding this comment.
Make it private because MetaData is package private.
Maybe make MetaData a private class too?
| @@ -271,9 +270,13 @@ public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOEx | |||
| public void fileRemoved(String filename) throws Exception; | |||
| } | |||
|
|
|||
| /** | |||
| * Notification of files that changed in the last scan. | |||
| * | |||
| _scanInterval = scanInterval; | ||
| schedule(); | ||
| } | ||
| { |
| } | ||
| schedule(); | ||
| } | ||
|
|
||
| public TimerTask newTimerTask() |
There was a problem hiding this comment.
Make it private and replace with Jetty's Scheduler.
| */ | ||
| @Deprecated | ||
| public void setRecursive(boolean recursive) | ||
| public void setFilenameFilter(FilenameFilter filter) |
There was a problem hiding this comment.
Still worth to keep around Deprecated method?
There was a problem hiding this comment.
I don't really want to mess with the WebAppProvider's filter for finding webapps to deploy at this late stage - it can be refactored at a later stage, so I've raised #5748 to track it.
Signed-off-by: Jan Bartel <[email protected]>
|
@sbordet I've done most if not all of things you requested so please rereview when ready. The failed build doesn't seem to have anything to do with this PR. |
Signed-off-by: Jan Bartel <[email protected]>
OOps, looks like it was actually a real problem, just heavily disguised in build paraphernalia. Should be fixed now. |
…5086-review-scanner
Signed-off-by: Jan Bartel <[email protected]>
More cleanups in code adding more privateness, getting rid of unnecessary exceptions, making fields final, etc. Signed-off-by: Simone Bordet <[email protected]>
|
@janbartel I went on and polished this PR even more, you may want to review my commit. |
| @@ -135,8 +135,7 @@ public void scan() | |||
|
|
|||
| _scanner.scan(callback); | |||
| _scanner.scan(callback); | |||
| complete.await(10, TimeUnit.SECONDS); | |||
|
|
|||
| return complete.await(10, TimeUnit.SECONDS); | |||
There was a problem hiding this comment.
@sbordet this will return true even if the callback fails, which is a bit strange.
There was a problem hiding this comment.
@gregw I don't understand? If the 10 s elapsed, complete.await() would have returned false, and the return value would have been ignored. Now we return it so that a JMX call to this method would know if it timed out.
There was a problem hiding this comment.
Scan is passed a callback. If it is failed then this method should return false. Moreover I think if it times out then I think this method should throw a timeout exception rather than return
There was a problem hiding this comment.
@sbordet ^
Also I don't like the 10second wait either, but not sure how to avoid other than infinite wait
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Closes #5086
The issue task was review the locking strategy for the Scanner. I've done that, made changes to hopefully remove any necessity for locking, and also removed some methods that were deprecated in jetty-9.4. As part of the refactoring of the code, there were some behaviour changes in the reporting of file changes, which I don't believe will make any substantive difference ( I can't even remember why I put those specific behaviours in there in the first place) but I'll call them out here anyway:
Note when reviewing: as a reminder of how the Scanner works, it is still the case that it takes at least 2 scan cycles to report a file as being ADDED: the first to detect the addition of the file, and the second to check that the file size or last modified time hasn't changed. Similarly, it takes at lesat 2 scan cycles to report a file as CHANGED: the first to detect the change, and the second to check the file size or last modified time hasn't changed.