Remove tasks from Elasticsearch when workflow is removed #3300

RemcoBuddelmeijer · 2022-10-17T19:12:52Z

Pull Request type

Bugfix
Feature
Refactoring (no functional changes, no api changes)
Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
WHOSUSING.md
Other (please describe):

NOTE: Please remember to run ./gradlew spotlessApply to fix any format violations.

Changes in this PR

Implement removal of tasks when calling /workflow/remove
Written tests for the new behaviour
- Introduced tests for WorkflowServiceTest
- Introduced tests for WorkflowResourceTest
- Introduced tests for ExecutionDAOFacadeTest
- Introduced tests for QueueResilienceSpec.groovy
- Introduced tests for TestElasticSearchRestDAOV6
- Introduced tests for TestElasticSearchDAOV6
Updated Javadocs where necessary to reflect on the new changes

Describe the new behavior from this PR, and why it's needed
Issue #1505

Tasks were not removed from Elasticsearch once the workflow was removed. This is necessary to not leave hanging objects in Elasticsearch that are not connected to any existing instance of a workflow. This change allows you to specify to also remove the tasks.

Alternatives considered

None

Additional notes

Options for removing tasks and archiving tasks have both been considered. In the final draft only the option to remove tasks was introduced. This to keep the changes clear and communicate to the user that tasks are removed now. On top of this all of this was done as it's unknown if the users are using this undefined behaviour of tasks not being removed. This was later on removed in the PR as it can be trusted that users do not rely on unidentified behaviour.

Right now to minimize the impact of the change the option to toggle on removing tasks or not is present. Only methods being called by the WorkflowResource class for workflow removal allows you to delete tasks, any other class such as WorkflowServiceImpl does not implement this yet. This can be done in a future PR.

* Add removeTask method for IndexDAO Implement removeTask and asyncRemoveTask methods to the IndexDAO interface. Implemented in ElasticSearchDAOV6, ElasticSearchRestDAOV6, and NoopIndexDAO. * Implement removal of tasks for removeWorkflow - Update ExecutionDAOFacade#removeWorkflow with two new parameters: removeTasks and archiveTasks - Added ExecutionDAOFacade#removeTaskIndex - Added methods updateTask and asyncUpdateWorkflow to IndexDAO - Added methods updateTask and asyncUpdateWorkflow to ElasticSearchDAOV6 and ElasticSearchRestDAOV6 * Add tests for removing workflow through /workflow/remove - Introduced tests for WorkflowServiceTest - Introduced tests for WorkflowResourceTest - Introduced test for ExecutionDAOFacadeTest - Introduced test for QueueResilienceSpec.groovy * Update Javadoc to include task archival when workflow archived and removeTasks true

RemcoBuddelmeijer · 2022-10-17T19:13:24Z

Once reviewed I will resolve the merge conflicts.

es6-persistence/src/main/java/com/netflix/conductor/es6/dao/index/ElasticSearchRestDAOV6.java

- Now tasks are search and associated with workflows through a search query directly embedded rather than constructed - If a task can not be removed it will error and return - Monitors are updated automatically - Updated TestElasticSearchRestDAOV6 and TestElasticSearchDAOV6

RemcoBuddelmeijer · 2022-10-17T21:41:02Z

I have gone ahead and resolved the merge conflicts @jxu-nflx @apanicker-nflx @ghoshabhi
Please allow the workflow to be ran so it can verify if all is still working as expected.

apanicker-nflx · 2022-10-17T21:50:42Z

@RemcoBuddelmeijer Thanks for creating the PR and 💯 on adding a wide slew of tests.

While I understand the reasoning for adding a flag removeTasks to the API and service layers, it should be the default behavior. The current implementation where tasks are left orphaned when workflow is removed is really a bug and your PR directly addresses this. Hence, I would suggest removing the flag and making it the default behavior.

core/src/main/java/com/netflix/conductor/core/dal/ExecutionDAOFacade.java

RemcoBuddelmeijer · 2022-10-17T21:58:08Z

@RemcoBuddelmeijer Thanks for creating the PR and 💯 on adding a wide slew of tests.

While I understand the reasoning for adding a flag removeTasks to the API and service layers, it should be the default behavior. The current implementation where tasks are left orphaned when workflow is removed is really a bug and your PR directly addresses this. Hence, I would suggest removing the flag and making it the default behavior.

@apanicker-nflx Okay that sounds good. I'll go ahead and remove it and trust on people not relying on undefined behaviour.

…flix#1505)

Currently ExecutionDAOFacade would call removeTask double the amount of times, this has been fixed. Now the only bug still present is that tasks are not terminated once removed and with that the index may also not be removed.

RemcoBuddelmeijer · 2022-10-18T00:13:47Z

@apanicker-nflx After updating the QueueResilienceSpec test to also include archiving workflows it notices that the workflow or the tasks associated with the workflow are not terminated in the same execution as this is done through the ExecutionService.
I would like to propose to keep this out of the testing plan since the QueueResiliencySpec is not to test the termination of workflows. We could always reintroduce it but force the status of the tasks and workflow. Right now I don't have the knowledge to determine if it is possible and then possibly be a bug once the termination of the workflow is not executed prior to the archival of the index.

At least for now I will push the change removing the archival test of QueueResiliencySpec.

I have pushed this as part of this PR to show the execution of both scenarios.

james-deee · 2022-10-18T15:55:01Z

core/src/main/java/com/netflix/conductor/core/dal/ExecutionDAOFacade.java

        try {
            removeWorkflowIndex(workflow, archiveWorkflow);
        } catch (JsonProcessingException e) {
            throw new TransientException("Workflow can not be serialized to json", e);
        }
-
-        executionDAO.removeWorkflow(workflowId);
+        tasks.forEach(


Why loop through the tasks twice? A regular for loop might be easier to read here, and couldn't you do both operations through a single loop of the tasks?

Personally I found it more readable and easier to have two separate loops with separate error handling where queue removal can be controlled separately from index removal.
When it comes to a traditional for loop: I wanted to keep in line with the style of the file. But this can always be changed.

Either way since it’s unnecessary to loop over something twice and now that the order of method has been changed allowing the loops to be merged, I’ll go ahead and merge the loops.

james-deee · 2022-10-18T15:59:17Z

The only other thing I would call out is that the Community Version will need to be updated as well for the V7 ElasticSearch IndexDAO. So whoever releases this fix (thanks for fixing btw!!!!!), will need to be aware when they bump the version over in Community @apanicker-nflx

RemcoBuddelmeijer · 2022-10-18T22:49:52Z

The only other thing I would call out is that the Community Version will need to be updated as well for the V7 ElasticSearch IndexDAO. So whoever releases this fix (thanks for fixing btw!!!!!), will need to be aware when they bump the version over in Community @apanicker-nflx

@james-deee I can always merge this logic into the community edition as well.

james-deee · 2022-11-17T15:43:11Z

@apanicker-nflx sorry for the ping, just wanted to see the status of this PR. Thanks!

ghoshabhi

LGTM

james-deee · 2022-12-02T00:19:49Z

@ghoshabhi and @apanicker-nflx when is an ETA that we can expect this to get merged and for a new release of Conudctor to go out?

I know there is also some batch polling that was added to the Java client that we'd also like to take advantage of. Thanks!

james-deee · 2023-01-06T17:50:24Z

@ghoshabhi or @apanicker-nflx sorry to ping again, just wanted to see if there was an update on this. Thanks!

ghoshabhi reviewed Oct 17, 2022

View reviewed changes

es6-persistence/src/main/java/com/netflix/conductor/es6/dao/index/ElasticSearchRestDAOV6.java Outdated Show resolved Hide resolved

RemcoBuddelmeijer requested a review from ghoshabhi October 17, 2022 21:25

jxu-nflx approved these changes Oct 17, 2022

View reviewed changes

apanicker-nflx mentioned this pull request Oct 17, 2022

Tasks not removed from ElasticSearch when deleting workflow #1505

Closed

Resolve merge conflicts

8b5969c

apanicker-nflx reviewed Oct 17, 2022

View reviewed changes

core/src/main/java/com/netflix/conductor/core/dal/ExecutionDAOFacade.java Show resolved Hide resolved

RemcoBuddelmeijer added 2 commits October 18, 2022 00:01

Removed rawJSON field from task archival from index DAO (Netflix#1505)

4ebe638

Removed removeTasks boolean parameter, now always removing tasks (Net…

3382975

…flix#1505)

RemcoBuddelmeijer requested review from apanicker-nflx and ghoshabhi and removed request for ghoshabhi and apanicker-nflx October 17, 2022 23:09

RemcoBuddelmeijer added 2 commits October 18, 2022 01:41

Update QueueResilienceSpec tests to include archive workflow

0ae8b3e

Update ExecutionDAOFacade to fix removeTask

e184068

Currently ExecutionDAOFacade would call removeTask double the amount of times, this has been fixed. Now the only bug still present is that tasks are not terminated once removed and with that the index may also not be removed.

Remove archival of workflow tasks from QueueResiliencySpec

19d6287

james-deee reviewed Oct 18, 2022

View reviewed changes

Merged loops for removing task index and queue removal (Netflix#1505)

fa7c210

ghoshabhi approved these changes Nov 23, 2022

View reviewed changes

apanicker-nflx approved these changes Jan 23, 2023

View reviewed changes

apanicker-nflx merged commit 0c30109 into Netflix:main Jan 23, 2023

RemcoBuddelmeijer deleted the feature/remove_tasks_through_workflow_resource branch January 26, 2023 12:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove tasks from Elasticsearch when workflow is removed #3300

Remove tasks from Elasticsearch when workflow is removed #3300

RemcoBuddelmeijer commented Oct 17, 2022 •

edited

Loading

RemcoBuddelmeijer commented Oct 17, 2022

RemcoBuddelmeijer commented Oct 17, 2022 •

edited

Loading

apanicker-nflx commented Oct 17, 2022

RemcoBuddelmeijer commented Oct 17, 2022 •

edited

Loading

RemcoBuddelmeijer commented Oct 18, 2022 •

edited

Loading

james-deee Oct 18, 2022

RemcoBuddelmeijer Oct 18, 2022 •

edited

Loading

james-deee commented Oct 18, 2022

RemcoBuddelmeijer commented Oct 18, 2022

james-deee commented Nov 17, 2022

ghoshabhi left a comment

james-deee commented Dec 2, 2022

james-deee commented Jan 6, 2023

Remove tasks from Elasticsearch when workflow is removed #3300

Remove tasks from Elasticsearch when workflow is removed #3300

Conversation

RemcoBuddelmeijer commented Oct 17, 2022 • edited Loading

Pull Request type

Changes in this PR

Alternatives considered

Additional notes

RemcoBuddelmeijer commented Oct 17, 2022

RemcoBuddelmeijer commented Oct 17, 2022 • edited Loading

apanicker-nflx commented Oct 17, 2022

RemcoBuddelmeijer commented Oct 17, 2022 • edited Loading

RemcoBuddelmeijer commented Oct 18, 2022 • edited Loading

james-deee Oct 18, 2022

Choose a reason for hiding this comment

RemcoBuddelmeijer Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

james-deee commented Oct 18, 2022

RemcoBuddelmeijer commented Oct 18, 2022

james-deee commented Nov 17, 2022

ghoshabhi left a comment

Choose a reason for hiding this comment

james-deee commented Dec 2, 2022

james-deee commented Jan 6, 2023

RemcoBuddelmeijer commented Oct 17, 2022 •

edited

Loading

RemcoBuddelmeijer commented Oct 17, 2022 •

edited

Loading

RemcoBuddelmeijer commented Oct 17, 2022 •

edited

Loading

RemcoBuddelmeijer commented Oct 18, 2022 •

edited

Loading

RemcoBuddelmeijer Oct 18, 2022 •

edited

Loading