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.

Sign up for GitHub

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

Jump to bottom

[JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co… #147

Merged

MarianMacik merged 18 commits into kiegroup:master from abhijithumbe:JBPM-9334

Oct 14, 2020

Contributor

abhijithumbe commented Sep 10, 2020

…mmand is long running.

Thank you for submitting this pull request

JIRA: (please edit the JIRA link if it exists)

referenced Pull Requests: (please edit the URLs of referenced pullrequests if they exist)

paste the link(s) from GitHub here
link 2
link 3 etc.

How to retest a PR or trigger a specific build:

* a pull request please add comment: Jenkins retest this
 
* a full downstream build please add comment: Jenkins run fdb
  
* a compile downstream build please  add comment: Jenkins run cdb

* a full production downstream build please add comment: Jenkins execute product fdb

* an upstream build please add comment: Jenkins run upstream


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

b7efa41

…mmand is long running.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

9 similar comments

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

kie-ci commented Sep 10, 2020

Can one of the admins verify this PR? Comment with 'ok to test' to start the build.

Member

krisv commented Sep 10, 2020

ok to test

tsurdilo requested changes

View reviewed changes

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated

@@ @@ -43,7 +47,8 @@ @@
                       icon = "Exec.png",
                       parameters = {
                               @WidParameter(name = "Command", required = true),
-                              @WidParameter(name = "Arguments", runtimeType = "java.util.List")
+                              @WidParameter(name = "Arguments", runtimeType = "java.util.List"),
+                              @WidParameter(name = "CommandExecutionTimeout", runtimeType = "java.lang.String")

Member

tsurdilo Sep 10, 2020 •

edited

Just "Timeout" would be simpler here, but not a big deal at all.
Also specifying the expected time value (seconds, minutes, hours) would be nice.
So if you expect seconds you could call it "TimeoutInSeconds" for example.

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated

@@ @@ -104,6 +131,10 @@ public void abortWorkItem(WorkItem workItem, @@
                                             WorkItemManager manager) {
                       // Do nothing, this work item cannot be aborted
                   }
+                  public void SetDefaultTimeoutInSeconds(long defaultTimeoutInSeconds) {

Member

tsurdilo Sep 10, 2020

method name should start with lower case letter.

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated

+              			String commandExecutionTimeout = (String) workItem.getParameter("CommandExecutionTimeout");
+              			if (commandExecutionTimeout ==null) {
+              				this.SetDefaultTimeoutInSeconds(4L);

Member

tsurdilo Sep 10, 2020

you can just initialize defaultTimeoutInSeconds up front with this value then you dont need to do it here.

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated

@@ @@ -71,6 +78,13 @@ public void executeWorkItem(WorkItem workItem, @@
                           String command = (String) workItem.getParameter("Command");
                           List<String> arguments = (List<String>) workItem.getParameter("Arguments");
+              			String commandExecutionTimeout = (String) workItem.getParameter("CommandExecutionTimeout");
+              			if (commandExecutionTimeout ==null) {

Member

tsurdilo Sep 10, 2020

commandExecutionTimeout is a string, so you should also check if its empty, and if it actually is a time.
Also if you are expecting the timeout to be in seconds, then the parameter should prob change to make sure thats clear, for example "TimeoutInSeconds" or something that is clear to readers what is expected.

You can also look into using ISO 8601 if possible to describe the time, for example "PT2M" for "2 minutes" etc.

Contributor Author

abhijithumbe Sep 13, 2020

done

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated

		@@ -82,11 +96,24 @@ public void executeWorkItem(WorkItem workItem,

		parsedCommandStr = commandLine.toString();

		ExecuteWatchdog watchdog = new ExecuteWatchdog(java.time.Duration.ofSeconds(defaultTimeoutInSeconds).toMillis());

Member

tsurdilo Sep 10, 2020

use import rather than full path (java.time.Duration)

Contributor Author

abhijithumbe Sep 13, 2020

done

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated

+              			    } catch (IOException e) {
+              			        if (watchdog.killedProcess()) {
+              			        	logger.debug("A timeout occured after "+java.time.Duration.ofSeconds(defaultTimeoutInSeconds).toMillis()+"ms while executing a command "+parsedCommandStr.replace(",", ""));
+              						results.put(RESULT, outputStream.toString());

Member

tsurdilo Sep 10, 2020 •

edited

If there was a timeout error / exception then I assume outputstream is empty ? wonder if you should return a default value or none at all here. Consider also how will the process know that there the execution of this command failed? Maybe a specific return value can be used that lets the rest of the process be able to reason over a possible failure.

Contributor Author

abhijithumbe Sep 13, 2020

outputstream do have a partial response for ping command. But sending partial response is not good idea. I have added failure message in result object in case of timeout and also added logger to log error.

exec-workitem/src/test/java/org/jbpm/process/workitem/exec/ExecWorkItemHandlerTest.java Outdated

                       assertNotNull(result);
                       assertTrue(result.contains("java version") || result.contains("jdk version"));
                   }
+                  @Test(timeout=6000)

Member

tsurdilo Sep 10, 2020

I dont understand how this test method actually tests the code that you have added in the handler :) please explain. Need a test that actually times out to test that the timeout is actually handled.

Contributor Author

abhijithumbe Sep 13, 2020

In this test I am executing ping command, when we execute ping command from the terminal we have to manually terminate the command. Here junit test will wait for 5 seconds to complete its executiom. If command execution didnt terminated/completed within 5 secs, test will keep running for its completion. So to terminate junit test added 6 seconds timeout for test.

Member

tsurdilo commented Sep 10, 2020

Thanks for the PR! Left some comments. Also for this PR and last one you did I wonder if the code could be formatted in your editor?


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

c5cebe6

…mmand is long running

Contributor Author

abhijithumbe commented Sep 13, 2020

Seems issue with IDE code format. Do we have any same code to includude in IDE formattter/code style ?
kindly review the new commit c5cebe6

abhijithumbe requested a review from tsurdilo

September 13, 2020 06:52

Member

tsurdilo commented Sep 13, 2020

@abhijithumbe One small thing I noticed is that in #145 it also has a timeout parameter that expects milliseconds and called "TimeoutInMillis" - do you think we could align the two so they are the same? Probably little confusing that one workitem expects milliseconds and the other seconds. WDYT?

Member

tsurdilo commented Sep 13, 2020

Thanks for the contribution and doing all the updates!
This looks good to me, the only thing is formatting, I think you can just follow this page for optaplanner :) - https://github.com/kiegroup/optaplanner/blob/master/ide-configuration/ide-configuration.adoc

Member

tsurdilo commented Sep 13, 2020

Jenkins retest this


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

f39cade

…mmand is long running

Contributor Author

abhijithumbe commented Sep 14, 2020

@abhijithumbe One small thing I noticed is that in #145 it also has a timeout parameter that expects milliseconds and called "TimeoutInMillis" - do you think we could align the two so they are the same? Probably little confusing that one workitem expects milliseconds and the other seconds. WDYT?

updated WIH to accept timeout in milliseconds.

Contributor Author

abhijithumbe commented Sep 14, 2020

Thanks for the contribution and doing all the updates!
This looks good to me, the only thing is formatting, I think you can just follow this page for optaplanner :) - https://github.com/kiegroup/optaplanner/blob/master/ide-configuration/ide-configuration.adoc

Thank you for all your help :) Now code should be in the required format. Kindly review latest commit

Member

tsurdilo commented Sep 21, 2020

@abhijithumbe LGTM

@MarianMacik same here- do we need a QE review or can merge ?

tsurdilo approved these changes

View reviewed changes

Member

tsurdilo commented Sep 21, 2020

Jenkins retest please

Contributor Author

abhijithumbe commented Sep 29, 2020

@abhijithumbe LGTM

@MarianMacik same here- do we need a QE review or can merge ?

@MarianMacik can you please share thoughts on this ?


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

90d129e

…mmand is long running

MarianMacik reviewed

View reviewed changes

Member

MarianMacik left a comment

Just formatting. Otherwise looks good. Thanks a lot for the improvement!

exec-workitem/src/main/java/org/jbpm/process/workitem/exec/ExecWorkItemHandler.java Outdated Show resolved Hide resolved

exec-workitem/src/test/java/org/jbpm/process/workitem/exec/ExecWorkItemHandlerTest.java Outdated Show resolved Hide resolved


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

5d01ab3

…mmand is long running

Contributor Author

abhijithumbe commented Oct 6, 2020

@MarianMacik Could you please review?

abhijithumbe added 2 commits

October 6, 2020 11:02


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

7814e8c

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

99b15a2

…mmand is long running

abhijithumbe requested a review from MarianMacik

October 7, 2020 05:58

abhijithumbe added 11 commits

October 7, 2020 16:00


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

5a11cfc

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

087cf79

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

e9d8746

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

7035dd6

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

0a87bcb

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

e4f580d

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

3cd9f2a

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

74d3b61

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

0218a2c

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

a8c0e74

…mmand is long running


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co…

65804ab

…mmand is long running

MarianMacik approved these changes

View reviewed changes

MarianMacik merged commit 0256e21 into kiegroup:master

matejonnet pushed a commit to matejonnet/jbpm-work-items that referenced this pull request


          [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if co… (

b06a281

kiegroup#147)

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running.

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

* [JBPM-9334] Process execution stuck at ExecWorkItemHandler node if command is long running

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment