-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Cam 4450 timer event listener : JobDefinition Changes #225
Cam 4450 timer event listener : JobDefinition Changes #225
Conversation
related to CAM-4450
related to CAM-4450
related to CAM-4450
… into CAM-4450-timer-event-listener
@@ -37,6 +43,11 @@ | |||
|
|||
public static final String[] CMMN_RESOURCE_SUFFIXES = new String[] { "cmmn11.xml", "cmmn10.xml", "cmmn" }; | |||
|
|||
protected static final PropertyMapKey<String, Map<String,TimerJobDeclaration<?>>> TIMER_EVENT_JOB_DECLARATIONS_PROPERTY = |
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.
Could you please change it as follows:
protected static final PropertyMapKey<String, List<JobDeclaration<?, ?>>> JOB_DECLARATIONS_PROPERTY =
new PropertyMapKey<String, List<JobDeclaration<?, ?>>>("JOB_DECLARATIONS_PROPERTY");
Hi Subhro, Additionally to the remarks above, I would suggest to implement it as follows:
Does it work for you? Thanks! Cheers, |
Hi Roman, Thanks, On Mon, Apr 25, 2016 at 1:06 PM, Roman Smirnov notifications@github.com
|
Hello Roman,
I also started with #2, from your earlier email, ie impl of TimerEventListenerJobHandler.execute method, let me know if this is an issue, i will revert my changes for this file. Thanks a lot for your time. |
@Override | ||
protected void definitionAddedToDeploymentCache(DeploymentEntity deployment, CaseDefinitionEntity definition, Properties properties) { | ||
List<JobDeclaration<?, ?>> declarations = properties.get(JOB_DECLARATIONS_PROPERTY).get(definition.getKey()); | ||
if(declarations!=null && !declarations.isEmpty())updateJobDeclarations(declarations, definition, deployment.isNew()); |
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.
Please follow our code styling and change it into:
if(declarations!=null && !declarations.isEmpty()) {
updateJobDeclarations(declarations, definition, deployment.isNew());
}
Hi Subhro,
In order to clean the DB you have to delete the created JobDefinition when the deployment gets deleted. Therefore you have to extend the
Therefore you have to implement
See my note above.
I would prefer to revert your changes regarding Thanks. Cheers, |
…DefByCaseId impl.
Hello Roman, Do I have to set some ActivityImpl along with TimerEventListenerJobDeclaration object in TimerEventListenerItemHandler.java? Let me know if there are any changes to the same. Thanks a lot for your time. |
Hi Subhro,
Good point! It is necessary to set the activity id on the
Then you should be able to set the activity on the As a consequence, the if-check in line Could you please adjust this? Thanks. Cheers, |
Hello Roman, I have done the changes as u mentioned. Let me know if all is ok. Thanks again for your time, On Thu, Apr 28, 2016 at 1:19 PM, Roman Smirnov notifications@github.com
|
d6aae5f
to
0e43e48
Compare
Hi Subhro, Sorry for my late response. I merged your changes into the branch I also updated the branch with current I just want to let you know that we are going to release 7.5 in the end of May 2016. It would be nice, if we could manage to add the timer event listener implementation to this release. Therefore the remaining points should be finished until next week (18th May), so that there is some time left to test it. Do you think we can manage it? Thanks for your effort! Cheers, |
Hello Roman, Will update my development branch CAM-4450-timer-event-listener. Cheers, On Tue, May 10, 2016 at 2:56 PM, Roman Smirnov notifications@github.com
|
Hello Roman, I have synced the master properly, but this feature branch I dont know, why I find Git so hard. :( Thanks a lot for your time, Cheers On Tue, May 10, 2016 at 4:05 PM, Subhrajyoti Moitra subhrajyotim@gmail.com
|
Hi Roman, I am seeing, 3 changed files. I thought, all this code was already Please guide. Thanks and regards, On Tue, May 10, 2016 at 4:43 PM, Subhrajyoti Moitra subhrajyotim@gmail.com
|
Hi Subhro, Have you tried the following:
Be aware that this will throw away all your local changes. If you want to keep your local changes, you can put them in a different branch first Does it help you? Cheers, |
Yes, Roman, this helps. But still i have diffs on the following files. Fast-forward The delete job def code are the changes. Starting on point#2 from your earlier email. Thanks, On Wed, May 11, 2016 at 12:19 PM, Roman Smirnov notifications@github.com
|
Hi Subhro, Yes, revert these changes. Cheers, |
Hi Roman, There is only a space. Hoping this wont create major issues. Thanks, On Wed, May 11, 2016 at 3:57 PM, Roman Smirnov notifications@github.com
|
Hello Roman, Thanks and regards, On Wed, May 11, 2016 at 6:16 PM, Subhrajyoti Moitra subhrajyotim@gmail.com
|
Hi Subhro,
I think this should not be a problem. I will take care of it when I will merge your pull request.
I think the implementation of public class TimerEventListenerJobHandler extends TimerEventJobHandler {
public void execute(TimerJobConfiguration configuration, CoreExecution context, CommandContext commandContext, String tenantId) {
CmmnExecution execution = (CmmnExecution) context;
execution.occur();
}
} The internals already contains for (almost) each transition defined in the specification an implementation. So, in that case we just have to call Regarding tests I would expect some test cases that the job has been triggered and the timer event listener "occured". Therefore you can define a
I really appreciate your effort and your motivation. Yet, please be aware that there is the risk that you will not be able to provide the complete solution in a way that we can merge it to master without changes. Since we are currently focusing on our 7.5 release, we would not be able to put much more effort in ourselves to make it mergeable. I do not want to discourage you, I just want to point this out now so that you are aware. Happy Hacking :) Cheers, |
@subhrajyotim I just want to say: Thank You! Cool that you are taking all the effort to implement this new feature. We are looking forward to using it, will be of great help. |
Thanks Jan for those encouraging words, trying my best to get this Cheers, On Thu, May 12, 2016 at 1:18 PM, Jan Galinski notifications@github.com
|
Hello Roman, I have made the changes and started the test case, I had to make a small if(execution!=null) jobHandler.execute(configuration, execution, Earlier this code had only the processExecution object. Let me know how to
I cannot get my CaseExecutionListener to be called. I have added the @deployment(resources = executeAvailableJobs(); Cmmn contents: R3/PT2SHow do I wait until the jobs are done, any pointers? JobEntity tests have Roman, no issues with your last point, if it does not make it in 7.5, i Thanks, On Thu, May 12, 2016 at 11:53 AM, Roman Smirnov notifications@github.com
|
Hi Subhro, Could you please create a new pull request with your current changes? Then it is easier to give you some advises. Thanks, |
Done Roman, Thanks a lot for your time, given the fact I know u are caught up with 7.5 Thanks and regards, On Fri, May 13, 2016 at 11:39 AM, Roman Smirnov notifications@github.com
|
Hello Roman,
Please review the changes. Please let me know what all changes are required.
Thanks a lot for your time.
Subhro.