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

Ensure non-started jobs are removed from jobs map when stopping #177

Merged
merged 1 commit into from
Mar 5, 2022

Conversation

svanegas
Copy link
Member

@svanegas svanegas commented Mar 4, 2022

The problem

Whenever a frame extraction gets cancelled by its request id, there were some cases when a subsequent extract call for the same request id was not reporting any result/error/cancellation.

The cause

The initial cancellation call was fired when the extraction job hadn't started yet, this was causing the job never being interrupted and never getting into a point of triggering a cancellation listener. The cancellation listener is the one in charge of removing the job from the activeJobMap, so since it was never removed, subsequent calls with the same request id were immediately discarded because the job still was in the activeJobMap.

The solution

Add a isStarted property to the FrameExtractJob, which gets set to true right at the beginning of the run method of the Runnable. This property is exposed to ComparableFutureTask, which at the same time can be read from the ActiveExtractJob in order to determine when the job is being cancelled but hasn't started yet.

Visuals

Here's an example of this behavior, modifying a bit the litr-demo we can observe the issue and after the fix.

  • Caching was removed for better visualization
  • A request id is not random but is uniquely mapped to each frame (based on the timestamp)
Before After
before after

Copy link
Contributor

@izzytwosheds izzytwosheds left a comment

Choose a reason for hiding this comment

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

Note to self: We should probably do something similar for TransformationJobs as well.

@izzytwosheds izzytwosheds merged commit c717770 into linkedin:main Mar 5, 2022
@svanegas svanegas deleted the fix/stop branch March 8, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants