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

Support loading workflow definitions from database #531

Merged
merged 26 commits into from
Jul 29, 2022

Conversation

gmokki
Copy link
Member

@gmokki gmokki commented Jun 3, 2022

The maximum rate can be controller with nflow.definition.loadMissingFromDatabaseSeconds - default rate is once every 60 seconds. But the definitions are only loaded from database if needed, not constantly in the background.

@gmokki gmokki requested a review from eputtone June 3, 2022 10:37
@gmokki gmokki force-pushed the on-demand-definiton-loading branch from 28fc406 to ffbe63b Compare June 3, 2022 11:40
@gmokki gmokki force-pushed the on-demand-definiton-loading branch from d99b6b7 to 7d9a191 Compare June 7, 2022 06:49
@efonsell
Copy link
Member

efonsell commented Jun 7, 2022

konffin default arvo pitäis olla default propsuissa ja sitten se voisikin olla required

Copy link
Member

@efonsell efonsell left a comment

Choose a reason for hiding this comment

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

update new config to readme

@gmokki gmokki force-pushed the on-demand-definiton-loading branch 6 times, most recently from 9d1d14a to 4bdfde2 Compare July 22, 2022 07:05
@gmokki gmokki changed the title Support loading workflow definitions from database if nflow.definition.load=true is set Support loading workflow definitions from database Jul 22, 2022
@gmokki gmokki force-pushed the on-demand-definiton-loading branch from b5918e6 to 01fb149 Compare July 23, 2022 04:43
@gmokki gmokki requested a review from efonsell July 26, 2022 11:00
Copy link
Member

@eputtone eputtone left a comment

Choose a reason for hiding this comment

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

Looks otherwise good, the comments require no action, but the implementation seems to miss handling for this kind of scenarios:

  • Two executors 1 & 2 in the same group, both have dispatcher running.
  • Executor 1 has implementation for WorkflowDefinition1
  • Executor 2 has implementation for WorkflowDefinition2
  • WorkflowDefinition1 instance is submitted - prevent it from being executed on executor 2

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated
@@ -14,6 +15,13 @@
**Details**

- `nflow-engine`
- Add support to manage (create, update and read, but not execute) workflow instances without having the workflow implementation classes in the classpath.
- If a workflow definition is not found from the classpath, it is loaded from the database and stored in memory.
- Workflow definitions that are loaded from the database are refreshed at most at configured interval (`nflow.definition.loadMissingFromDatabaseSeconds`, default 60).
Copy link
Member

Choose a reason for hiding this comment

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

Theoretically, someone might depend on the fact that instances cannot be managed for historical workflow definitions. So need to consider which kind of version needs to be released next, or could start with -1 as default and change the default to 60 in next major version?

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 find that a bit too theoretical to worry about breaking it. Nevertheless, I added few lines on readme about that and instructions on how to disable the new functionality.


private static WorkflowState getInitialState(StoredWorkflowDefinition stored) {
return stored.states.stream()
.filter(state -> start.name().equals((state.type)))
Copy link
Member

Choose a reason for hiding this comment

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

It a shame that we don't currently persist the default start state (=initial state). But can't be helped here, perhaps add another issue about that.

@gmokki gmokki merged commit 4ba6bb2 into master Jul 29, 2022
@gmokki gmokki deleted the on-demand-definiton-loading branch July 29, 2022 12:20
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.

3 participants