Skip to content

Conversation

@Excommunicated
Copy link
Contributor

This is for Issue #105
Added attribute based on the discussion in the listed issue..
Please let me know if there is anything I have misinterpreted.

@odinserj
Copy link
Member

Awesome! And sorry for huge delay. I'll incorporate this changes to 1.1.2.

@odinserj
Copy link
Member

Oh, can you merge these changes with the upstream (I can't automatically merge this PR)?

@Excommunicated
Copy link
Contributor Author

Merged upstream and CI build is passing.

@ses4j
Copy link

ses4j commented Oct 2, 2014

Nice work, and thanks - we needed this feature. One small question, though: "Latency" doesn't seem like the right word in the DeleteOnLatencyTimeoutAttribute. What do you think about something such as ProcessingTimeLimit? Processing describes the state it is in during this period, tying it into the verbiage from the frontend website.

@ses4j
Copy link

ses4j commented Oct 2, 2014

Also, not to complicate things, but you may want to think ahead to implementing soft and hard timeouts. A soft timeout would set the cancellation token to "cancel yourself!" and a hard timeout would terminate the worker. See, for example, celery's design: http://celery.readthedocs.org/en/latest/userguide/workers.html#time-limits

@Excommunicated
Copy link
Contributor Author

I'll let Sergey make the decision on the naming of the attribute. As for your other ideas, Once this gets merged, you should submit another issue so discussion can happen. It sounds like a good idea though..

@keith-kurak
Copy link

Are there any plans to merge in this change? This sounds really useful. Or is there another way to fail a job after a specified timespan has been exceeded?

@odinserj odinserj added this to the 1.6.0 milestone Jun 10, 2016
odinserj pushed a commit that referenced this pull request Jun 10, 2016
* Add xmldocs for the DeleteOnLatencyTimeoutAttribute class
* Make timeout value required via ctor parameter
* Fix compile-time errors appeared after merging with upstream
* Fixing Breaks
* Fix for latency attribute unit test failure
* Add Latency Filter Attribute To Delete job if timeout is exceeded
@odinserj
Copy link
Member

Okay, this filter was finally merged into Hangfire under the LatencyTimeoutAttribute name. It really acts on the basis of latency, and applied to a background job that is going to be processed right now (and this is not the processing time limit), so the naming is fine.

@Excommunicated, thank you for this, and for patience 👍 ❤️ I've merged it using git command line, preserving your authorship (03a3414), so I'm closing this PR instead of merging it.

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

Development

Successfully merging this pull request may close these issues.

4 participants