Fix #327: Change machine states by scheduler #331
Conversation
| */ | ||
| @Transactional(readOnly = true) | ||
| public void changeMachineStatesInBatch() { | ||
| try (Stream<IdentityVerificationEntity> stream = identityVerificationService.streamAllIdentityVerificationsToChangeState().parallel()) { |
There was a problem hiding this comment.
Do we want to use .parallel()?
| final String processId = identityVerification.getProcessId(); | ||
| final OwnerId ownerId = new OwnerId(); | ||
| ownerId.setActivationId(identityVerification.getActivationId()); | ||
| ownerId.setUserId(identityVerification.getUserId()); |
There was a problem hiding this comment.
👀 Needed to have the real userId from IdentityVerificationEntity especially for the presence check provider.
| * | ||
| * @return identity verifications | ||
| */ | ||
| @Query("SELECT id FROM IdentityVerificationEntity id WHERE" + |
There was a problem hiding this comment.
Maybe we should consider a refactor of existing DocumentsVerificationSyncTask and ClientEvaluationSyncTask tasks and cover all status = IN_PROGRESS here. But I see this would require adding new actions to the machine. So rather postpone.
Additionally some transitions from the ACCEPTED states could be maybe switched automatically by the state machine to the next state.
There was a problem hiding this comment.
I tried to limit where clause as much as possible. I may think about ACCEPTED once again.
There was a problem hiding this comment.
I recalled, we need to keep them because another jobs such as DocumentsVerificationSyncTask. We may consolidate it in an extra PR/issue.
| @@ -123,6 +134,33 @@ public Message<OnboardingEvent> createMessage(OwnerId ownerId, String processId, | |||
| .build(); | |||
| } | |||
|
|
|||
| /** | |||
| * | |||
| " AND id.status = com.wultra.app.enrollmentserver.model.enumeration.IdentityVerificationStatus.ACCEPTED)" + | ||
| " OR (id.phase = com.wultra.app.enrollmentserver.model.enumeration.IdentityVerificationPhase.PRESENCE_CHECK" + | ||
| " AND id.status = com.wultra.app.enrollmentserver.model.enumeration.IdentityVerificationStatus.IN_PROGRESS)" | ||
| ) |
There was a problem hiding this comment.
What about some sorting of the result verifications? oldest first?
There was a problem hiding this comment.
Do we need it? I mean, in that case we should have an index over updated time, right?
| /** | ||
| * | ||
| */ | ||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
I am not sure about duration of the transaction here. Maybe we should limit the timeout with some generous value, but I am pretty sure we can expect that the stream will have an end.
| ownerId.setUserId(identityVerification.getUserId()); | ||
| logger.debug("Changing state of machine for process ID: {}", processId); | ||
|
|
||
| transactionTemplate.setPropagationBehavior(TransactionDefinition.PROPAGATION_REQUIRES_NEW); |
There was a problem hiding this comment.
I would expect using the shedlock to acquire an exclusive right over the state machine here (shedlock name based on processId?)
There was a problem hiding this comment.
But for sure exclusive streaming of all waiting verifications by one instance and checking is also a way to go.
| /** | ||
| * Scheduled task to change machine state. | ||
| */ | ||
| @Scheduled(cron = "${enrollment-server-onboarding.state-machine.changeMachineState.cron:0/1 * * * * *}", zone = "UTC") |
There was a problem hiding this comment.
I am aware of only simple cron patterns. Maybe there is some magic which I don't see here.
What's the frequency of this cron, every second? Is there a difference with * * * * * *?
Maybe we can use e.g. */3.
There was a problem hiding this comment.
The default value seems to be inconsistent with the value set in properties file.
There was a problem hiding this comment.
Ok, we may change it to three seconds, but all cron expressions in enrollment using zero, like 0/5, which should be same.
| @Transactional(readOnly = true) | ||
| public void changeMachineStatesInBatch() { | ||
| try (Stream<IdentityVerificationEntity> stream = identityVerificationService.streamAllIdentityVerificationsToChangeState().parallel()) { | ||
| stream.forEach(this::changeMachineState); |
There was a problem hiding this comment.
It would be nice to add the number of processed identity verifications per batch, probably in debug level.
No description provided.