KOGITO-530/531 Jobs persistence and retries on error #2

tiagodolphine · 2019-11-16T11:46:10Z

Initial implementation of persistence with infinipsan and retries on errors.

To disable persistence you can set the property on application.properties persistence=false or use -Dpersistence=false on the startup, in this way it will use the in-memory repository.

mswiderski

Added few comments but lets have a chat tomorrow so I better understand what it does and how :)

mswiderski · 2019-11-17T20:19:48Z

...ns/jobs/jobs-service/src/main/java/org/kie/kogito/jobs/service/executor/HttpJobExecutor.java

@@ -39,34 +47,71 @@
    private static final Logger LOGGER = LoggerFactory.getLogger(HttpJobExecutor.class);

    @Inject
-    Vertx vertx;
+    private Vertx vertx;


Use package protected as otherwise quarkus reports warnings

Great point, I was not aware of this behavior. I changed the all attributes with @Inject until all warnings were gone.

mswiderski · 2019-11-17T20:22:42Z

addons/jobs/jobs-service/src/main/java/org/kie/kogito/jobs/service/model/ScheduledJob.java


+@RegisterForReflection


Don’t think this is needed, we won’t run it as naitive executable

Yeah, my bad I forgot to remove it.
Done

mswiderski · 2019-11-17T20:26:08Z

...service/src/main/java/org/kie/kogito/jobs/service/repository/impl/JobRepositoryDelegate.java

+
+    @Inject
+    public JobRepositoryDelegate(@Any Instance<ReactiveJobRepository> instances,
+                                 @ConfigProperty(name = "persistence")


We need to prefix the config ptoperty so it is clear what it refers to ‚kogito.job-service.persistence’ or similar

Yes, I agree.
Done

mswiderski · 2019-11-17T20:27:07Z

...src/main/java/org/kie/kogito/jobs/service/repository/infinispan/InfinispanConfiguration.java

+
+    @Inject
+    public InfinispanConfiguration(Instance<RemoteCacheManager> cacheManagerInstance,
+                                   @ConfigProperty(name = "persistence")


Same here, prefix the config property

Yes, I agree.
Done

mswiderski · 2019-11-17T20:29:48Z

...src/main/java/org/kie/kogito/jobs/service/repository/infinispan/InfinispanJobRepository.java

+import static org.kie.kogito.jobs.service.repository.infinispan.InfinispanConfiguration.Caches.SCHEDULED_JOBS;
+
+@ApplicationScoped
+@Repository(persistence = true)


Do we really need this qualifier? Is this to allow to have in memory and infinispan in the same app?

The idea here was to have multiple implementations of the interface, for instance one for in-memory and other for infinispan, in this way injecting the interface on beans cause exceptions, so the solution was to have only one @Default impl that delegates to the concrete class according to the config property for persistence. So I added the qualifier to solve this problem, do you have a better/simpler idea? I'll be happy to change, this was my last change to solve the problem to disable or enable persistence.
One observation, since the injection is Lazy, the infinispan client is not being initialized by quarkus that why I'm injecting the Instance<ReactiveJobRepository> when persistence is not enabled, it evicts to get Exception during the startup.

how about using named injections and then let people configure in application property which one to use? It will be more explicit and we could easily add more implementations. While the default would be in memory and we only look up beans when the config property is given. That way we could move all the code related to bootstrapping into the given bean that will be initialised only when that property is set.

so implementation wise it would be similar ... still using qualifier (though we could use @Named but it is considered less CDI) but instead of linking it with persistence we could just have one that is @Repository("inmemory") and another one @Repository("infinispan")

and then in application.properties we could then have kogito.job-service.repository=infinispan and by default it will be kogito.job-service.repository=inmemory

wdyt?

and in case we would to this we don't have to specify if the persistence is enabled or nor as it will be taken based on the repository implementation anyway

mswiderski · 2019-11-17T20:32:32Z

...n/java/org/kie/kogito/jobs/service/repository/infinispan/marshaller/MarshallersProducer.java

+//    @Produces
+//    public EnumMarshaller statusMarshaller() {
+//        return new StatusMarshaller();
+//    }


Maybe remove if this is not needed

great catch.
Done

mswiderski · 2019-11-17T20:33:02Z

addons/jobs/jobs-service/src/main/java/org/kie/kogito/jobs/service/resource/JobResource.java

 @Path("/job")
 public class JobResource {

    private static final Logger LOGGER = LoggerFactory.getLogger(JobResource.class);

    @Inject
-    VertxJobScheduler scheduler;
+    private VertxJobScheduler scheduler;


Same here use package protected

mswiderski · 2019-11-17T20:35:03Z

.../jobs-service/src/main/java/org/kie/kogito/jobs/service/scheduler/BaseTimerJobScheduler.java

-    private Logger logger = LoggerFactory.getLogger(BaseTimerJobScheduler.class);
+    private static final Logger LOGGER = LoggerFactory.getLogger(BaseTimerJobScheduler.class);
+    public static final long BACKOFF_RETRY_MILLIS = TimeUnit.SECONDS.toMillis(1);
+    public static final long MAX_INTERVAL_LIMIT_TO_RETRY_MILLIS = TimeUnit.SECONDS.toMillis(60);


Shouldn’t this be configurable via application.properties

Yes, I agree, better to have it configurable.

mswiderski · 2019-11-17T20:39:07Z

addons/jobs/jobs-service/src/main/resources/application.properties

+quarkus.swagger-ui.path=/swagger-ui
+
+#Infinispan - more specific configs on hotrod-client.properties file.


You can actually use this file for hotrod auth setup which I would prefer as its easier to override

yeah, I'll move the config to application.properties.

Initial Job Service implementation inserting swagger

mswiderski · 2019-11-18T12:07:24Z

...ls/drools-core/src/main/java/org/drools/core/base/WrappedStatefulKnowledgeSessionForRHS.java

    @Override
    public JobsService getJobsService() {
        return delegate.getJobsService();
->>>>>>> further work on timer service


ouch ... looks like conflict not resolved?

tiagodolphine · 2019-11-18T12:11:17Z

@mswiderski I rebased with last changes on your fork, the conflicts should be solved.

…jobs when load job on startup

tiagodolphine · 2019-11-19T00:20:26Z

@mswiderski just pushed some changes, now the repository config is based on a string (the impl type), jobs are removed when achieving a final state, and some other improvements on onde.

* KOGITO-18 - Pluggable timer/jobs service that can be used as service * Initial Job Service implementation Initial Job Service implementation inserting swagger * Apply PR comments and some code refactoring, test * Fix JobScheduler when cancelling job * Fixing cancel already scheduled job when re-scheduling * Job retry implementation * Inserting Infinispan persistence on Job service * Inserting Infinispan persistence on Job service * fix infinispan repository * Inserting persistence selector based on config property * Fix scheduler manager and infinispan repository to work with config * Fix JobResourceTest * Removed fixed version on pom * Update plugin on job-service pom * Fix comments on the PR * Job repository type by config, remove jobs on final state, add retry jobs when load job on startup

mswiderski reviewed Nov 17, 2019

View reviewed changes

mswiderski force-pushed the job-service branch from 3914061 to 66334c6 Compare November 18, 2019 07:19

mswiderski and others added 15 commits November 18, 2019 09:01

KOGITO-18 - Pluggable timer/jobs service that can be used as service

aa4a263

Initial Job Service implementation

fc5b932

Initial Job Service implementation inserting swagger

Apply PR comments and some code refactoring, test

7f4e1af

Fix JobScheduler when cancelling job

a3d612d

Fixing cancel already scheduled job when re-scheduling

09bb01e

Job retry implementation

7d673b1

Inserting Infinispan persistence on Job service

9e38ec7

Inserting Infinispan persistence on Job service

b95a688

fix infinispan repository

503d9b7

Inserting persistence selector based on config property

180067a

Fix scheduler manager and infinispan repository to work with config

261aa84

Fix JobResourceTest

94caef4

Removed fixed version on pom

fcd0b32

Update plugin on job-service pom

75bac74

Fix comments on the PR

b3b22bf

mswiderski reviewed Nov 18, 2019

View reviewed changes

tiagodolphine force-pushed the KOGITO-530 branch from a27cabb to b3b22bf Compare November 18, 2019 12:08

Job repository type by config, remove jobs on final state, add retry …

6db3372

…jobs when load job on startup

mswiderski merged commit e0e174f into mswiderski:job-service Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KOGITO-530/531 Jobs persistence and retries on error #2

KOGITO-530/531 Jobs persistence and retries on error #2

tiagodolphine commented Nov 16, 2019

mswiderski left a comment

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 18, 2019

mswiderski Nov 18, 2019

tiagodolphine Nov 19, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 17, 2019

tiagodolphine Nov 18, 2019

mswiderski Nov 18, 2019

tiagodolphine commented Nov 18, 2019

tiagodolphine commented Nov 19, 2019

		quarkus.swagger-ui.path=/swagger-ui

		#Infinispan - more specific configs on hotrod-client.properties file.


		@RegisterForReflection

KOGITO-530/531 Jobs persistence and retries on error #2

KOGITO-530/531 Jobs persistence and retries on error #2

Conversation

tiagodolphine commented Nov 16, 2019

mswiderski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagodolphine commented Nov 18, 2019

tiagodolphine commented Nov 19, 2019