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

Release 3.0.0 #32

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Conversation

vivianzheng404
Copy link

@vivianzheng404 vivianzheng404 commented Jun 23, 2023

This version is compatible to conductor-boot v3.13.5
Besides mySQL, additionally supports PostgreSQL for persistent storage

@jas34 jas34 self-requested a review July 3, 2023 10:57
Copy link
Owner

@jas34 jas34 left a comment

Choose a reason for hiding this comment

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

Apart from inline comments, the following are the general comments:

  • Add author in new files as well as add since={version}.
  • Update the author with your name in all of the touched files.
  • We need to align of code style/format. If needed I can provide one which has been originally used in the older releases.
  • Update README.md for the addition of Postgres persistence layer. Currently, it mentions only MySQL implementation.

README.md Outdated Show resolved Hide resolved
pom.xml Show resolved Hide resolved
pom.xml Outdated
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.1</version>
<groupId>org.junit.jupiter</groupId>
Copy link
Owner

Choose a reason for hiding this comment

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

Conductor test cases have been built using junit. Any reason for upgrading from junit to junit-jupiter?
If not then I would recommend to stick to the dependency used by conductor.

Copy link
Author

@vivianzheng404 vivianzheng404 Jul 12, 2023

Choose a reason for hiding this comment

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

Run mvn --settings settings.xml -P bintray clean install

junit returns error

[ERROR] TestEngine with ID 'junit-vintage' failed to discover tests
[ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: There was an error in the forked process
[ERROR] TestEngine with ID 'junit-vintage' failed to discover tests
[ERROR] at org.apache.maven.plugin.surefire.booterclient.ForkStarter.fork(ForkStarter.java:631)
[ERROR] at org.apache.maven.plugin.surefire.booterclient.ForkStarter.run(ForkStarter.java:285)
[ERROR] at org.apache.maven.plugin.surefire.booterclient.ForkStarter.run(ForkStarter.java:250)

While org.junit.jupiter, build is successful.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

I have tried to run the mvn --settings settings.xml -P bintray clean install with dependency junit instead of org.junit.jupiter and the build was successful. I am attaching build logs also. Can you recheck some system-level or application-level settings?
build_sucess_logs_with_junit.txt

Copy link
Author

Choose a reason for hiding this comment

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

junit works when I add dependency junit-vintage-engine version 5.7.0+. Version 5.6.2 as your build logs shows, does not work. Refer to https://stackoverflow.com/questions/70452633/org-junit-platform-commons-junitexception-testengine-with-id-junit-jupiter-fa/74501390#74501390

The newer version of junit-vintage-engine is 5.10.0 https://repo1.maven.org/maven2/org/junit/vintage/junit-vintage-engine/ Besides junit, I am going to add dependency junit-vintage-engine version 5.10.0.

</dependency>


<dependency>
Copy link
Owner

Choose a reason for hiding this comment

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

This dependency should be only needed in the module scheduled-postgres-persistence just like conductor-mysql-persistence dependency inside scheduled-mysql-persistence

Copy link
Author

@vivianzheng404 vivianzheng404 Jul 12, 2023

Choose a reason for hiding this comment

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

Without conductor-postgres-persistence dependency here, I got error

java.lang.NoClassDefFoundError: com/netflix/conductor/common/run/WorkflowTestRequest
at java.lang.Class.getDeclaredConstructors0(Native Method) ~[?:?]
at java.lang.Class.privateGetDeclaredConstructors(Class.java:3137) ~[?:?]
at java.lang.Class.getDeclaredConstructors(Class.java:2357) ~[?:?]
at org.springframework.boot.context.properties.ConfigurationPropertiesBindConstructorProvider.findConstructorBindingAnnotatedConstructor(ConfigurationPropertiesBindConstructorProvider.java:62) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.context.properties.ConfigurationPropertiesBindConstructorProvider.getBindConstructor(ConfigurationPropertiesBindConstructorProvider.java:48) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.context.properties.ConfigurationPropertiesBean$BindMethod.forType(ConfigurationPropertiesBean.java:311) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.context.properties.ConfigurationPropertiesBeanDefinitionValidator.validate(ConfigurationPropertiesBeanDefinitionValidator.java:63) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.context.properties.ConfigurationPropertiesBeanDefinitionValidator.postProcessBeanFactory(ConfigurationPropertiesBeanDefinitionValidator.java:45) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:291) ~[spring-context-5.2.7.RELEASE.jar:5.2.7.RELEASE]
at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:175) ~[spring-context-5.2.7.RELEASE.jar:5.2.7.RELEASE]
at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:707) ~[spring-context-5.2.7.RELEASE.jar:5.2.7.RELEASE]
at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:533) ~[spring-context-5.2.7.RELEASE.jar:5.2.7.RELEASE]
at org.springframework.boot.web.servlet.context.ServletWebServerApplicationContext.refresh(ServletWebServerApplicationContext.java:143) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:758) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:750) [spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.SpringApplication.refreshContext(SpringApplication.java:397) [spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.SpringApplication.run(SpringApplication.java:315) [spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.SpringApplication.run(SpringApplication.java:1237) [spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at org.springframework.boot.SpringApplication.run(SpringApplication.java:1226) [spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
at io.github.jas34.scheduledwf.ScheduledWorkflowConductor.main(ScheduledWorkflowConductor.java:34) [classes/:?]
Caused by: java.lang.ClassNotFoundException: com.netflix.conductor.common.run.WorkflowTestRequest
at jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581) ~[?:?]
at jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178) ~[?:?]
at java.lang.ClassLoader.loadClass(ClassLoader.java:522) ~[?:?]

Copy link
Author

Choose a reason for hiding this comment

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

Can you make it work without adding conductor-postgres-persistence here?

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the information. I will work for sure work on this item.

Copy link
Owner

Choose a reason for hiding this comment

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

After removing this dependence from parent pom:

  1. I am able to build.
  2. I am able to run the application.

Can you elaborate you are getting this error on the application start or while running testes?

Copy link
Author

@vivianzheng404 vivianzheng404 Aug 12, 2023

Choose a reason for hiding this comment

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

After removing conductor-postgres-persistence from parent pom, the application is able to start, but it cannot function as expected.

  • Is the application started?
    Yes. IntelliJ UI shows the app is running.

The console shows this error message in the beginning:

org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'workflowSweeper' defined in URL [jar:file:/Users/vivianzheng/.m2/repository/com/netflix/conductor/conductor-core/3.13.5/conductor-core-3.13.5.jar!/com/netflix/conductor/core/reconciliation/WorkflowSweeper.class]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'workflowExecutor' defined in URL [jar:file:/Users/vivianzheng/.m2/repository/com/netflix/conductor/conductor-core/3.13.5/conductor-core-3.13.5.jar!/com/netflix/conductor/core/execution/WorkflowExecutor.class]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'deciderService' defined in URL [jar:file:/Users/vivianzheng/.m2/repository/com/netflix/conductor/conductor-core/3.13.5/conductor-core-3.13.5.jar!/com/netflix/conductor/core/execution/DeciderService.class]: Unsatisfied dependency expressed through constructor parameter 4; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'systemTaskRegistry' defined in URL [jar:file:/Users/vivianzheng/.m2/repository/com/netflix/conductor/conductor-core/3.13.5/conductor-core-3.13.5.jar!/com/netflix/conductor/core/execution/tasks/SystemTaskRegistry.class]: Unsatisfied dependency expressed through constructor parameter 0; nested exception is org.springframework.beans.factory.UnsatisfiedDependencyException: Error creating bean with name 'START_WORKFLOW' defined in URL [jar:file:/Users/vivianzheng/.m2/repository/com/netflix/conductor/conductor-core/3.13.5/conductor-core-3.13.5.jar!/com/netflix/conductor/core/execution/tasks/StartWorkflow.class]: Unsatisfied dependency expressed through constructor parameter 1; nested exception is org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'defaultValidator' defined in class path resource [org/springframework/boot/autoconfigure/validation/ValidationAutoConfiguration.class]: Invocation of init method failed; nested exception is javax.validation.ValidationException: Unable to parse null at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:797) ~[spring-beans-5.2.7.RELEASE.jar:5.2.7.RELEASE] at org.springframework.beans.factory.support.ConstructorResolver.autowireConstructor(ConstructorResolver.java:227) ~[spring-beans-5.2.7.RELEASE.jar:5.2.7.RELEASE]

then console keeps generating the same response:

Caused by: java.sql.SQLException: HikariDataSource HikariDataSource (HikariPool-1) has been closed. at com.zaxxer.hikari.HikariDataSource.getConnection(HikariDataSource.java:96) ~[HikariCP-3.4.5.jar:?] at com.netflix.conductor.postgres.dao.PostgresBaseDAO.getWithTransaction(PostgresBaseDAO.java:117) ~[conductor-postgres-persistence-3.13.5.jar:3.13.5] at com.netflix.conductor.postgres.dao.PostgresBaseDAO.lambda$getWithRetriedTransactions$3(PostgresBaseDAO.java:145) ~[conductor-postgres-persistence-3.13.5.jar:3.13.5] at org.springframework.retry.support.RetryTemplate.doExecute(RetryTemplate.java:329) ~[spring-retry-1.3.0.jar:?] at org.springframework.retry.support.RetryTemplate.execute(RetryTemplate.java:209) ~[spring-retry-1.3.0.jar:?] at com.netflix.conductor.postgres.dao.PostgresBaseDAO.getWithRetriedTransactions(PostgresBaseDAO.java:145) ~[conductor-postgres-persistence-3.13.5.jar:3.13.5] ... 9 more

  • Does the app function as expected?
    No. None of the endpoints works. Error: connect ECONNREFUSED ....:8080.

Also, the swagger UI refused to connect. localhost:8080/swagger-ui.html


Tests are successful. mvn --settings settings.xml -P bintray clean install This command runs fine.

Copy link
Author

@vivianzheng404 vivianzheng404 Aug 12, 2023

Choose a reason for hiding this comment

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

To reproduce this, try schedule some workflows, then shut down some or all workflows. From the repeating error message java.sql.SQLException: HikariDataSource HikariDataSource (HikariPool-1) has been closed and postgres.dao.PostgresBaseDAO.getWithTransaction, I think you may need to have some data/scheduled workflows in Postgres to reproduce.

Copy link
Author

Choose a reason for hiding this comment

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

In my case, I have 3 shutdown scheduled workflow, 1 running workflow.

  • No conductor-postgres-persistence from parent pom
    Behavior described as above.

  • With conductor-postgres-persistence from parent pom
    The console keeps generating the same log

86405 [pool-8-thread-1] DEBUG io.github.jas34.scheduledwf.execution.DefaultSchedulerManager [] - No workflow left to be scheduled. 86406 [pool-8-thread-1] DEBUG io.github.jas34.scheduledwf.execution.DefaultSchedulerManager [] - No running process found for shutdown with managerRef=fc656dcd-ed26-468e-b741-4b9da5b03910, with names=[check-conductor-health-workflow2, check-conductor-health-workflow, check-conductor-health-workflow3]

The console is flooded with repeating message, if you feel like this needs future improvement, can you create a work item?


<dependency>
<groupId>org.springframework.retry</groupId>
<artifactId>spring-retry</artifactId>
Copy link
Owner

Choose a reason for hiding this comment

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

  • This dependency should not be declared explicitly.
  • This dependency is needed by modules scheduled-mysql-persistence and scheduled-postgres-persistence.
  • We still do not need to declare this in these modules because they will be available transitively from conductor-mysql-persistence and conductor-postgres-persistence respectively.

Copy link
Author

Choose a reason for hiding this comment

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

If you remove the spring-retry block here, you will see error "java: package org.springframework.retry.support does not exist". Code won't run.

Without the dependency here, the
import org.springframework.retry.support.RetryTemplate; will not work.

Copy link
Author

Choose a reason for hiding this comment

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

If you use IntelliJ IDE, you can try to comment out spring-retry dependency, click, File -> Invalidate Caches -> Select all boxes, after the IDE reopens, and see if the code runs. I see error "java: package org.springframework.retry.support does not exist"

Copy link
Author

Choose a reason for hiding this comment

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

Can you make it work without adding spring-retry here?

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the information. I will work for sure work on this item.

Copy link
Owner

Choose a reason for hiding this comment

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

Your findings are correct. This dependency is not available from conductor modules because everywhere it has been defined with scope compileOnly. Check this line

We should do the following:

  1. This dependency is mostly relevant for persistence modules. We shall not declare these in parent pom.
  2. Let's add this dependency in each module - scheduled-mysql-persistence and scheduled-postgres-persistence.
  3. Conductor is using version 1.3.3, we shall also opt for the same.

Copy link
Author

@vivianzheng404 vivianzheng404 Aug 13, 2023

Choose a reason for hiding this comment

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

Removing spring-retry from parent pom and adding to scheduled-mysql-persistence and scheduled-postgres-persistence do not work.

The application failed to start, error message:


com.netflix.conductor.core.config.ConductorCoreConfiguration.onTransientErrorRetryTemplate(ConductorCoreConfiguration.java:118)

The following method did not exist:

org.springframework.retry.support.RetryTemplateBuilder org.springframework.retry.support.RetryTemplate.builder()


From the error message, ConductorCoreConfiguration class from package com.netflix.conductor.core.config uses RetryTemplate.

spring-retry should not be removed from parent pom. I updated the version to be 1.3.3.

scheduledwf-module/pom.xml Outdated Show resolved Hide resolved
scheduledwf-module/pom.xml Outdated Show resolved Hide resolved
.addParameter(def.getStatus() != null ? def.getStatus().name() : null)
.addJsonParameter(def).addParameter(def.getCronExpression()).executeUpdate());
} else {
// @formatter:off
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this comment from here as well as MySQLScheduledWfMetaDataDao.java#L110

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@vivianzheng404
Copy link
Author

Apart from inline comments, the following are the general comments:

  • Add author in new files as well as add @SInCE.
  • Update the author with your name in all of the touched files.
  • We need to align of code style/format. If needed I can provide one which has been originally used in the older releases.
  • Update README.md for the addition of Postgres persistence layer. Currently, it mentions only MySQL implementation.

Reply:

@jas34 jas34 self-requested a review July 13, 2023 05:56
pom.xml Outdated
<groupId>junit</groupId>
<artifactId>junit</artifactId>
<version>4.13.1</version>
<groupId>org.junit.jupiter</groupId>
Copy link
Owner

Choose a reason for hiding this comment

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

I have tried to run the mvn --settings settings.xml -P bintray clean install with dependency junit instead of org.junit.jupiter and the build was successful. I am attaching build logs also. Can you recheck some system-level or application-level settings?
build_sucess_logs_with_junit.txt


<dependency>
<groupId>org.springframework.retry</groupId>
<artifactId>spring-retry</artifactId>
Copy link
Owner

Choose a reason for hiding this comment

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

Your findings are correct. This dependency is not available from conductor modules because everywhere it has been defined with scope compileOnly. Check this line

We should do the following:

  1. This dependency is mostly relevant for persistence modules. We shall not declare these in parent pom.
  2. Let's add this dependency in each module - scheduled-mysql-persistence and scheduled-postgres-persistence.
  3. Conductor is using version 1.3.3, we shall also opt for the same.

</dependency>


<dependency>
Copy link
Owner

Choose a reason for hiding this comment

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

After removing this dependence from parent pom:

  1. I am able to build.
  2. I am able to run the application.

Can you elaborate you are getting this error on the application start or while running testes?

@@ -61,6 +66,14 @@
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter</artifactId>
<version>${spring-boot-version}</version>

<exclusions>
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the correct findings. Let's keep the exclusion and add spring-boot-starter-log4j2 which is also used by conductor-server.

@@ -21,7 +20,7 @@
* Date: 26/09/21-12:09 pm
*
* @since v2.0.0
* @author Jasbir Singh
* @author Jasbir Singh Vivian Zheng
Copy link
Owner

Choose a reason for hiding this comment

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

I believe there is no change in this file, please remove this change.

Copy link
Author

Choose a reason for hiding this comment

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

Removed


/**
* Description:<br>
* Date: 26/09/21-5:13 pm
* @since v2.0.0
* @author Jasbir Singh
* @author Jasbir Singh Vivian Zheng
Copy link
Owner

Choose a reason for hiding this comment

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

Use either of the formats to add author:

  • @author Jasbir Singh, Vivian Zheng - use coma ',' in between names
    or
  • @author Jasbir Singh \n @author Vivian Zheng - new line in between names.

Copy link
Author

@vivianzheng404 vivianzheng404 Aug 13, 2023

Choose a reason for hiding this comment

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

Added add new line and since.

@@ -51,14 +51,14 @@ public class DefaultSchedulerManager implements SchedulerManager {

@Autowired
public DefaultSchedulerManager(ScheduledWfMetadataDAO scheduledWfMetadataDAO,
ScheduledProcessRegistry processRegistry, MetadataDAO metadataDAO, IndexScheduledWfDAO indexDAO,
WorkflowSchedulingAssistant schedulingAssistant) {
ScheduledProcessRegistry processRegistry, MetadataDAO metadataDAO, IndexScheduledWfDAO indexDAO,
Copy link
Owner

Choose a reason for hiding this comment

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

Code format change

Copy link
Author

Choose a reason for hiding this comment

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

Removed the space.

@jas34
Copy link
Owner

jas34 commented Jul 15, 2023

Apart from inline comments, the following are the general comments:

  • Add author in new files as well as add @SInCE.
  • Update the author with your name in all of the touched files.
  • We need to align of code style/format. If needed I can provide one which has been originally used in the older releases.
  • Update README.md for the addition of Postgres persistence layer. Currently, it mentions only MySQL implementation.

Reply:

  • Even I am not aware of the person who got tagged (:P). My intention was to add Since for Java docs.
  • I am referring to formatting change like this.

@jas34 jas34 self-requested a review August 13, 2023 13:53
@vivianzheng404 vivianzheng404 changed the title Release 2.0.2 Release 3.0.0 Aug 13, 2023
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