Deps: allow any scheduler version with plugin #132
Conversation
This reverts commit b6505a0.
This reverts commit 793801a.
| s.add_runtime_dependency "logstash-core-plugin-api", ">= 1.60", "<= 2.99" | ||
| s.add_runtime_dependency 'logstash-codec-plain' | ||
| s.add_runtime_dependency "logstash-mixin-http_client", ">= 7.1.0" | ||
| s.add_runtime_dependency 'stud', "~> 0.0.22" |
There was a problem hiding this comment.
stud dependency was not used - there was one line which seemed as a reminder of some relic.
lib/logstash/inputs/http_poller.rb
Outdated
| if @scheduler && @scheduler.thread && @scheduler.thread.status | ||
| @scheduler.shutdown # on newer Rufus (3.8) this joins on the scheduler thread | ||
| end | ||
| # TODO implement client.close as we as releasing it's pooled resources! |
There was a problem hiding this comment.
I've a doubt on this, if in the .travis.yml and Gemfile we pin to Rufus 3.0.9 because LS 7.x pins that version, is this code 7.x proof?
Do this change behaves the same way also with Rufus 3.0.9 ?
There was a problem hiding this comment.
the missing client.close is yet another completely separate issue (basically a plugin leak on reload) - stumbled upon it while trying to investigate the CI failure - please ignore the TODO for now if it's confusing.
I've a doubt on this, if in the
.travis.ymlandGemfilewe pin to Rufus 3.0.9 because LS 7.x pins that version, is this code 7.x proof?
it's proof that tests run (on CI) against latest rufus-scheduler as well as 3.0.9. LS 7.x for now used 3.0.9 but in theory (when the plugin unpinning from elastic/logstash#12931 is complete) with the right scenario of multpile bin/logstash-plugin update ... the lock could be released and rufus could be updated.
previous version had @scheduler.shutdown if @scheduler, in theory this could still fail if the thread did not start or the thread died - in those cases we can not trigger a "safe" scheduler.shutdown. but this isn't smt we're seeing and I can revert the code to be the same as before if you have doubts (that change isn't that important atm).
There was a problem hiding this comment.
I'm fine with this, thanks for clarification.
Co-authored-by: Andrea Selva <[email protected]>
| opts = schedule_type == "every" ? { :first_in => 0.01 } : {} | ||
| @scheduler.send(schedule_type, schedule_value, opts) { run_once(queue) } | ||
| @scheduler.join | ||
| @scheduler.thread.join # due newer rufus (3.8) doing a blocking operation on scheduler.join |
There was a problem hiding this comment.
in rufus 3.0.9 @scheduler.join is ~ @scheduler.thread.join.
however in latest rufus 3.8 the @scheduler.join does a blocking Queue#pop so this makes it behave the same across versions ...
3 participants
Removing dependency lock (see elastic/logstash#12931):
CI 🔴 happens after the unpinning but is likely related to elastic/logstash#13572The 🔴 CI does reproduce sporadically locally but most of the time running Dockers as well as non-Docker
rspecis 🟢 on CI, since the new ubuntu base image, it fails pretty much all the time.There's a separate investigation going on at #130 (this PR was crafted from the other one thus commits are a bit verbose but the changelog is mostly around refactoring tests here). Also, please note the added target to keep testing against
3.0.9it's failing the same and confirms the issue is not related to the scheduler upgrade.There some test changes included (to make sure scheduler is always shutdown between tests).