Skip to content
This repository has been archived by the owner on Jan 3, 2024. It is now read-only.

fix: Invoke destroy handler in batch mode #32

Merged
merged 2 commits into from
Feb 12, 2021

Conversation

appbak3r
Copy link
Contributor

Description

Invoke destroy handler for project dependent tasks after batch task was finished.

Fixes #31

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests to cover my changes
  • I have made corresponding changes to the documentation

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement

@appbak3r appbak3r force-pushed the fix/invoke-destroy-batch-mode branch from 5cdcb4e to 63a3449 Compare December 29, 2020 15:37
} else {
if (!watcherStarted) {
for (const meta of metaByAction.values()) {
meta?.onDestroyHandler?.();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I logged meta here for some batch actions and they didn't have an onDestroyHandler.

I put the following code in runActionsInBatch and it seemed to work, what do you think?:

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, @beshanoe what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stephan-noel I dont get why would you assign a to the batchMeta the first encountered action's meta? probably you can assign there only a batch's onDestroyHandler? Can you please push into this PR a whole solution?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beshanoe you're right, I was only considering my own use case in which I need context.longRunning to only be called once per runner.

I guess maybe there are several approaches here:

  • Call context.longRunning once per runner (NOT per project per runner) like I do above. If you want to handle shutdown for several projects, you can do something like (ignore the invocation of onetime below) :

image

  • Call context.longRunning once per action (the approach you were expecting of per project per runner), and if the cleanup had already been done by a previous project (ie, the projects shared the same port and I already closed it), then I have to detect that it was already cleaned up or limit it the invocations somehow.

Honestly the first approach seemed more natural to me, but I may be biased to my use case, what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beshanoe Changed it a bit to separate the non-batch meta from batchMeta. I do set and get for batchMetaByAction where I do to bring attention to the fact that having one meta per action instead of per runner is a possibility in the future. What do you think?

@stephan-noel stephan-noel force-pushed the fix/invoke-destroy-batch-mode branch 3 times, most recently from 40c62fe to a719d6b Compare February 11, 2021 11:15
@appbak3r appbak3r force-pushed the fix/invoke-destroy-batch-mode branch from a719d6b to 3119df4 Compare February 11, 2021 13:39
@stephan-noel stephan-noel force-pushed the fix/invoke-destroy-batch-mode branch from 3119df4 to ee26e03 Compare February 11, 2021 20:32
@stephan-noel stephan-noel force-pushed the fix/invoke-destroy-batch-mode branch from ee26e03 to b5d1efe Compare February 12, 2021 11:14
@beshanoe beshanoe merged commit 132c6fc into Farfetch:master Feb 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch mode does not trigger longRunning onDestroy handlers
3 participants