-
Notifications
You must be signed in to change notification settings - Fork 95
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
[Feature] Handling multiple schedules in periodic manager - first version #1497
Conversation
34799cf
to
04e52c7
Compare
...rc/main/scala/pl/touk/nussknacker/engine/management/periodic/PeriodicPropertyExtractor.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please look for my comments.
...t/periodic/src/main/resources/db/batch_periodic/migration/hsql/V1_104__add_schedule_name.sql
Outdated
Show resolved
Hide resolved
...c/src/main/scala/pl/touk/nussknacker/engine/management/periodic/PeriodicProcessService.scala
Outdated
Show resolved
Hide resolved
...cala/pl/touk/nussknacker/engine/management/periodic/db/PeriodicProcessDeploymentsTable.scala
Show resolved
Hide resolved
95fdfd4
to
5b7bdea
Compare
5b7bdea
to
cc3aab1
Compare
...c/src/main/scala/pl/touk/nussknacker/engine/management/periodic/PeriodicProcessService.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one naming issue I've found. I know that it will need some migration but IMO it will cost us less if we do it now then spend time talking why it is named that way.
...eriodic/src/main/scala/pl/touk/nussknacker/engine/management/periodic/PeriodicProperty.scala
Outdated
Show resolved
Hide resolved
...eriodic/src/main/scala/pl/touk/nussknacker/engine/management/periodic/PeriodicProperty.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but let's someone with more production experiences take a look
90ef8e7
to
35b2a66
Compare
419ac40
to
2398c2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It's possible to define multiple named schedules (e.g. run with different parameters). Currently we don't handle running multiple Flink jobs with same ProcessName well, so there is limitation - we schedule only one process simultaneously (other schedules for same process have to wait).