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

Fix: compatibility with all (>= 3.0) rufus-scheduler versions #97

Merged
merged 14 commits into from Mar 14, 2022

Conversation

kares
Copy link
Contributor

@kares kares commented Dec 15, 2021

The work here is meant to get the plugin working with up-to-date rufus-scheduler versions.

ATM, LS 7.x is stuck with gem version 3.0.9 (due plugins pinning ~> 3.0.9), see elastic/logstash#12931

The previous rufus-scheduler < 3.5 lock (got tightened in version 3.2.2 to ~> 3.0.9), was due Rufus moving the Rufus::Scheduler::CronLine class into a standalone gem (Fugit::Cron) with a slightly changed API (removed #days method). That feature was only used for logging and is now replaced by simply logging the calculated frequency of the cron expression.

Also, part of the PR our custom Scheduler sub-class was reviewed for re-use with a new API draft to use across plugins (as we plan to extract the Scheduler mixin bits into its own gem), the idea being using a factory method to provide everything in one step e.g.

      @scheduler = ...::Scheduler.
          start_cron_scheduler(schedule, thread_name: "[#{id}]<...") { do_periodic_work(queue) }
      @scheduler.join

Most plugins will only need ^^^ this, the JDBC plugin still rely on some Rufus::Scheduler api (not worth to push the scheduler externalization that much further atm).

NOTE: we should consider not using any 3.1 - 3.5 version which had stopped using tzinfo gem for a self invented Rufus::ZoTime class, tzinfo is (transitively) used again in version >= 3.6 through an added gem.

@kares kares changed the title Experiment: try out latest rufus scheduler Fix: compatibility with all (>= 3.0) rufus-scheduler versions Jan 17, 2022
@kares kares marked this pull request as ready for review February 23, 2022 09:42
@kares kares requested review from jsvd and robbavey February 28, 2022 15:07
@kares kares requested review from andsel and removed request for jsvd March 2, 2022 10:51
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM!

A nice work in moving the Scheduler code in a common mixin

@kares kares merged commit 6a75566 into logstash-plugins:main Mar 14, 2022
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