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

enhance and cleanup the internal timers #100

Merged
merged 21 commits into from Dec 28, 2022

Conversation

querdenker2k
Copy link
Collaborator

@querdenker2k querdenker2k commented Dec 1, 2022

What I've done:

  • removed "getTimedLock" don't know what it was good for?
  • remove the timer impl from JRule and put it to a new class JRuleTimerHandler -> the calls are in JRule for timers are "proxied" to the new class
  • allowed unique timer, where no name must be set (easier to handle when using timers inside loops)
  • cleanup up in general the timer handling
  • set MDC tags and log name for timers as well
  • add JRuleDebounce to add a debounce for method calls

@seaside1
Copy link
Owner

seaside1 commented Dec 1, 2022

Get timed locked is used for checking if there is a timer running. I'm using it for blocking access.
For instance I have a power all off physical button, but I don't want it to be pushed multiple times (i.e my kids tend to push it 10 times in a row for fun).

I still think it's better to use completable futures as it is less overhead and also it's possible to chain the timers.

I was working on changing the timers to work better with chaining.

Instead of having:

time1= 10
createOrReplaceTimer(... time1) // Do something after 10 seconds
Launch next timer with offset
createOrReplaceTimer(time1+3) // Do something else after 13 seconds
you could chain them

createOrReplaceTimer(time).createOrReplaceAdditionalTimer(//Will be invoked when first timer finishes)

Other than that it seems good. I would like to keep the timedLock (See example 2: https://github.com/seaside1/jrule#example-2

And I would like to be able to Chain the timers.

/S

@querdenker2k
Copy link
Collaborator Author

yes ok, makes all sense.

@querdenker2k
Copy link
Collaborator Author

@seaside1 Can you please help with the current error in the pipeline. I've no dependencies touched and have no idea of the osgi stuff.

@seaside1
Copy link
Owner

seaside1 commented Dec 3, 2022

I've added deps for netty let see if it works. Locally it works, but some tests still fail

@querdenker2k
Copy link
Collaborator Author

Got it. Used a netty dependency in the code where i don't want to.

@seaside1
Copy link
Owner

seaside1 commented Dec 8, 2022

I'll have a look shortly

@seime
Copy link
Collaborator

seime commented Dec 8, 2022

The README file should probably be reviewed as well

@querdenker2k
Copy link
Collaborator Author

The README file should probably be reviewed as well

done

@seaside1
Copy link
Owner

I'm failing to build this locally. I get

[ERROR] Message: Unable to resolve root: missing requirement [root] osgi.identity; osgi.identity=openhab-automation-jrule; type=karaf.feature; version=3.4.0.SNAPSHOT; filter:="(&(osgi.identity=openhab-automation-jrule)(type=karaf.feature)(version>=3.4.0.SNAPSHOT))" [caused by: Unable to resolve openhab-automation-jrule/3.4.0.SNAPSHOT: missing requirement [openhab-automation-jrule/3.4.0.SNAPSHOT] osgi.identity; osgi.identity=org.openhab.automation.jrule; type=osgi.bundle; version="[3.4.0.202212142049,3.4.0.202212142049]"; resolution:=mandatory [caused by: Unable to resolve org.openhab.automation.jrule/3.4.0.202212142049: missing requirement [org.openhab.automation.jrule/3.4.0.202212142049] osgi.wiring.package; filter:="(osgi.wiring.package=com.oracle.svm.core.annotate)"]]

I think this caused by the Debounce class pulling some new dependency .

Can you guys build locally?

@seaside1
Copy link
Owner

Build is working for me now. Unclear if it was java 17 vs java 11 problem, or simply my checkout that was bad.

@seaside1
Copy link
Owner

I have been having a lot lately, give me some time and I'll test and verify against my prod. I really like what you have done in this pr.

@querdenker2k
Copy link
Collaborator Author

add some integration-tests for an easier decision and a faster merge ;)

@seime
Copy link
Collaborator

seime commented Dec 28, 2022

@querdenker2k could you rebase and resolve conflicts? I know we have too many open (large) PRs and I am trying to merge some of them.

…ance_cleanup_timers

# Conflicts:
#	README.md
#	src/main/java/org/openhab/automation/jrule/internal/engine/JRuleEngine.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemChangeExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemReceivedCommandExecutionContext.java
#	src/main/java/org/openhab/automation/jrule/internal/engine/excutioncontext/JRuleItemReceivedUpdateExecutionContext.java
@querdenker2k querdenker2k reopened this Dec 28, 2022
@seime seime mentioned this pull request Dec 28, 2022
@seime seime self-requested a review December 28, 2022 18:06
@seime seime merged commit efa8e32 into seaside1:main Dec 28, 2022
@querdenker2k querdenker2k deleted the enhance_cleanup_timers branch December 28, 2022 20:55
@seaside1
Copy link
Owner

Good to see this merged. I have been sick for the past weeks and was just about done testing this.

querdenker2k added a commit to querdenker2k/jrule that referenced this pull request Feb 11, 2023
Co-authored-by: Joseph Hagberg <joseph@zoidberg.se>
Co-authored-by: Arne Seime <arne.seime@gmail.com>
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

3 participants