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.

Sign up for GitHub

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

Jump to bottom

Refactor WatchService #3004

Merged
merged 8 commits into from
Feb 12, 2023
Merged

Refactor WatchService #3004

merged 8 commits into from
Feb 12, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Jun 16, 2022 •
edited
Loading

Depends on #2994

This replaces the AbstractWatchService with a new WatchService interface and implementation that can be consumed as OSGi service. It makes use of a library that does a lot of what we currently do (hashing, removing duplicate events) and also implements a native watch service on MacOS which decreases response time to file changes.

@wborn Since we discussed this issue in the past, WDYT about the design?

@J-N-K J-N-K added work in progress A PR that is not yet ready to be merged afterRelease labels Jun 16, 2022
@wborn wborn mentioned this pull request Jun 18, 2022
Closed
@J-N-K J-N-K force-pushed the feature-macwatch branch 2 times, most recently from 43bdc1a to 74d29ef Compare June 18, 2022 16:51
@J-N-K J-N-K mentioned this pull request Jun 26, 2022
Merged
@J-N-K J-N-K force-pushed the feature-macwatch branch 2 times, most recently from e8fa89a to 17e24a7 Compare July 31, 2022 16:44
@wborn wborn mentioned this pull request Aug 20, 2022
Merged
@J-N-K J-N-K added enhancement An enhancement or new feature of the Core awaiting other PR Depends on another PR and removed work in progress A PR that is not yet ready to be merged labels Aug 20, 2022
@J-N-K J-N-K marked this pull request as ready for review August 20, 2022 13:53
@J-N-K J-N-K requested a review from a team as a code owner August 20, 2022 13:53
@J-N-K J-N-K added this to the 4.0 milestone Aug 20, 2022
@J-N-K J-N-K mentioned this pull request Nov 18, 2022
Closed
@J-N-K J-N-K mentioned this pull request Dec 4, 2022
Merged
jlaur
jlaur reviewed Dec 15, 2022
View reviewed changes
bom/compile/pom.xml Outdated Show resolved Hide resolved
@J-N-K J-N-K mentioned this pull request Dec 15, 2022
Closed
5 tasks
This was referenced Dec 18, 2022
Merged
Merged
@wborn wborn removed the awaiting other PR Depends on another PR label Dec 19, 2022
@wborn wborn removed this from the 4.0 milestone Dec 19, 2022
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-4-0-snapshot-discussion/142322/22

@florian-h05
Copy link
Contributor

@J-N-K What is the state here? I mean how much finished is this one?

@J-N-K
Copy link
Member Author

J-N-K commented Dec 20, 2022

Code-wise complete, but I'm pretty sure there are some minor tweaks needed.

@J-N-K J-N-K mentioned this pull request Jan 5, 2023
Merged
@wborn wborn removed the awaiting other PR Depends on another PR label Jan 13, 2023
@J-N-K
Copy link
Member Author

J-N-K commented Jan 14, 2023

One of the minor tweaks is to fix this test:

TEST org.openhab.core.model.core.internal.folder.FolderObserverTest#testModification()

I refactored it to a unit test. We want to test the FolderObserver, not the WatchService. It's clearly a timing issue, as it sometimes occurs, sometimes not and never on my local builds.

@J-N-K
Copy link
Member Author

J-N-K commented Jan 14, 2023

@wborn This seems to be ready now.

wborn
wborn requested changes Jan 15, 2023
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good in general! 👍
I do already have a few minor review comments. 🙂

...nhab/core/automation/module/script/rulesupport/internal/loader/DefaultScriptFileWatcher.java Outdated
Comment on lines 48 to 52
@Activate
@Override
public void activate() {
super.activate();
}

@Deactivate
@Override
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove @Override if it still overrides the parent method?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed the methods, the activate/deactivate methods are called automatically.

.../org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java Outdated Show resolved Hide resolved
...penhab/core/automation/module/script/rulesupport/loader/AbstractScriptDependencyTracker.java Outdated Show resolved Hide resolved
.../org/openhab/core/automation/module/script/rulesupport/loader/AbstractScriptFileWatcher.java Outdated Show resolved Hide resolved
@J-N-K
Add WatchService
0ee3df2 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
spotless
bf95f17 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
fix
c64a27a 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
refactor folderobservertest
9123592 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
improvement
e40946d 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
resolve itests
31a0e2d 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
address review comments
be8877b 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
address review comments
d3cf7c3 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@florian-h05
Copy link
Contributor

@wborn
FYI, I think your review was addressed.

@florian-h05
Copy link
Contributor

@openhab/core-maintainers Would be nice if this gets merged soon. Thanks!

wborn
wborn approved these changes Feb 12, 2023
View reviewed changes
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks!

@wborn wborn merged commit 7f113c5 into openhab:main Feb 12, 2023
@wborn wborn added this to the 4.0 milestone Feb 12, 2023
@J-N-K J-N-K deleted the feature-macwatch branch February 12, 2023 13:12
@J-N-K J-N-K mentioned this pull request Feb 14, 2023
Merged
This was referenced Feb 18, 2023
Closed
Merged
@jimtng jimtng mentioned this pull request Feb 26, 2023
Closed
@wborn wborn mentioned this pull request Apr 1, 2023
Merged
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
@J-N-K @splatch
Refactor WatchService (openhab#3004)
6fd4e70 
Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: 7f113c5
@wborn wborn mentioned this pull request Jul 30, 2023
Open
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh-4-1-1-startup-takes-more-than-an-hour/153310/64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlaur jlaur jlaur left review comments

@wborn wborn wborn approved these changes

Assignees
No one assigned
Labels
API breaking enhancement An enhancement or new feature of the Core
Projects
openHAB 4 Issue Tracking
Archived in project
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Rules are sometimes loaded two times on edit
5 participants
@J-N-K @openhab-bot @florian-h05 @wborn @jlaur