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

IQSS/7517-fix-prepublication-workflow #7519

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Jan 20, 2021

What this PR does / why we need it: In earlier code, the call to FinalizeDatasetPublicationCommand was only called from the PublishDatasetCommand when there was no pre-publication workflow. In the workflow case, FinalizeDatasetPublicationCommand is called when the workflow completes. When the call to Finalize publication was moved to the onsuccess method, it ended up executing even when there is a pre-pub workflow which means a) it is called twice, and b) it is potentially executing in parallel with the workflow (async ones at least).

Which issue(s) this PR closes:

Closes #7517

Special notes for your reviewer:

Suggestions on how to test this:
DANS will be testing. A relatively simple test would be to configure a workflow with the 'pause' step as a pre-publication workflow and to verify that the dataset is not published until after the command to unpause is sent. (Should also verify that doing this workflow without the PR ends up publishing the dataset even without un-pausing the worklflow.)

Note: from the conversation below, there is currently an issue with the 'pause' step being worked on separately. While it is not necessary to test with it, it does simplify testing a bit, so we suggest waiting until that is merged to test this PR.

Does this PR introduce a user interface change? If mockups are available, please link/include them here: none

Is there a release notes update needed for this change?:

Additional documentation:

@coveralls
Copy link

coveralls commented Jan 20, 2021

Coverage Status

Coverage decreased (-0.001%) to 19.622% when pulling e5f7fb8 on GlobalDataverseCommunityConsortium:IQSS/7517-fix-prepublication-workflows into bd91444 on IQSS:develop.

@janvanmansum
Copy link
Contributor

janvanmansum commented Jan 21, 2021

@qqmyers : I have tried testing our own workflow, but it contains a bug, which has to be solved first, so I'll get back to you about that.

I am now trying to test a minimal scenario, as you suggested.

Workflow "pause.json":

{
  "name": "Pause",
  "steps": [
    {
       "provider":":internal",
       "stepType":"log",
       "parameters": {
           "invocation ID": "${invocationId}" 
        }
    },{
      "provider":":internal",
      "stepType":"pause"
    }
  ]
}

I have added this step and set it as the default for PrePublishDataset:

curl http://localhost:8080/api/admin/workflows --data-binary @pause.json -H 'Content-Type: application/json'
{"status":"OK","data":{"name":"Pause","id":4,"steps":[{"stepType":"log","provider":":internal","parameters":{"invocation ID":"${invocationId}"}},{"stepType":"pause","provider":":internal"}]

curl -X PUT --data 4 http://localhost:8080/api/admin/workflows/default/PrePublishDataset
{"status":"OK","data":{"message":"Default workflow id for trigger PrePublishDataset set to 4"}}

I then created and published a dataset and it gets locked and stays locked.

However I have trouble resuming it. I tried to log the invocationId using the variable that is passed to the workflow, but apparently that doesn't work. Fortunately, it is logged anyway by the log step:

[#|2021-01-21T12:56:23.257+0100|INFO|Payara 5.2020|edu.harvard.iq.dataverse.workflow.internalspi.LoggingWorkflowStep|_ThreadID=297;_ThreadName=__ejb-thread-pool11;_TimeMill
is=1611230183257;_LevelValue=800;|
  Logging step:|#]

[#|2021-01-21T12:56:23.257+0100|INFO|Payara 5.2020|edu.harvard.iq.dataverse.workflow.internalspi.LoggingWorkflowStep|_ThreadID=297;_ThreadName=__ejb-thread-pool11;_TimeMill
is=1611230183257;_LevelValue=800;|
  Invocation id e94f6da1-8969-4d46-a848-3dcc5357ce7b|#] <============== INVOCATION ID HERE =======

[#|2021-01-21T12:56:23.257+0100|INFO|Payara 5.2020|edu.harvard.iq.dataverse.workflow.internalspi.LoggingWorkflowStep|_ThreadID=297;_ThreadName=__ejb-thread-pool11;_TimeMill
is=1611230183257;_LevelValue=800;|
  Dataset id:10|#]

[#|2021-01-21T12:56:23.257+0100|INFO|Payara 5.2020|edu.harvard.iq.dataverse.workflow.internalspi.LoggingWorkflowStep|_ThreadID=297;_ThreadName=__ejb-thread-pool11;_TimeMill
is=1611230183257;_LevelValue=800;|
  Trigger Type PrePublishDataset|#]

[#|2021-01-21T12:56:23.258+0100|INFO|Payara 5.2020|edu.harvard.iq.dataverse.workflow.internalspi.LoggingWorkflowStep|_ThreadID=297;_ThreadName=__ejb-thread-pool11;_TimeMill
is=1611230183258;_LevelValue=800;|
  Next version:1.0 isMinor:false|#]

I then tried to resume e94f6da1-8969-4d46-a848-3dcc5357ce7b:

curl -X POST http://localhost:8080/api/workflows/e94f6da1-8969-4d46-a848-3dcc5357ce7b
{"status":"ERROR","message":"Cannot find workflow invocation with id e94f6da1-8969-4d46-a848-3dcc5357ce7b"}

Any ideas?

@qqmyers
Copy link
Member Author

qqmyers commented Jan 21, 2021

I'll check - it looks #6672 is a separate issue. I'll look into it.

@qqmyers
Copy link
Member Author

qqmyers commented Jan 21, 2021

@janvanmansum Some progress - It looks like the log is not showing the right invocationid. If I look in the pendingworkflowinovcation table and use the id there, things work. So - I'll look into why the wrong id is being logged.

@qqmyers
Copy link
Member Author

qqmyers commented Jan 21, 2021

@janvanmansum - #7521 should address the issue of not being able to resume a paused workflow. If another issue surfaces, let me know.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Review 🦁 to QA 🔎✅ Jan 21, 2021
@scolapasta scolapasta removed their assignment Jan 21, 2021
IQSS/7517-fix-prepublication-workflows

Conflicts:
	src/main/java/edu/harvard/iq/dataverse/workflow/WorkflowServiceBean.java
@kcondon kcondon merged commit 9d6801d into IQSS:develop Jan 21, 2021
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 Jan 21, 2021
@djbrooke djbrooke added this to the 5.4 milestone Jan 21, 2021
@janvanmansum
Copy link
Contributor

@qqmyers : Above scenario is indeed fixed with #7521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Pre-publication workflows fail
6 participants