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

Remove obsolete enqueue_front option #1887

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alxckn
Copy link

@alxckn alxckn commented Nov 21, 2023

b45a685 had introduced a enqueue_front option which enabled LIFO job execution instead of FIFO.

But a refactoring in 4bb4441 ignored that change.

This is a proposal to remove this unused property to avoid confusion about it not being functional.

@PatrickTulskie
Copy link
Contributor

@alxckn do you think maybe it makes sense to just fix the functionality instead of deleting the property?

@alxckn
Copy link
Author

alxckn commented Nov 22, 2023

@PatrickTulskie I think this functionality was active in the master branch only for a couple of months 7 years ago.

So there does not seem to be any use for it at the moment. At least not in this way where the whole application is configured to use LIFO over FIFO.
I know some comments mentioned this type of functionality could be useful at a queue level or even at a "per enqueue" level (I could be interested in contributing on this aspect).

Which is why I believe that the feature as it is is better removed to avoid confusion when reading the code docs.

@PatrickTulskie
Copy link
Contributor

If it is getting removed, since the method has been around for a while, it's probably best to deprecate it properly and then remove it in the next major release.

That said, since the method is there and it's possible people out there are making calls to it and are expecting it to work, it might be better to just fix the bug. It does seem like a useful feature.

@alxckn
Copy link
Author

alxckn commented Nov 22, 2023

You're probably right about being more careful to deprecate the method in steps.

Your comment makes me think of another argument against fixing this issue. If I had activated this option without noticing it does nothing, I wouldn't want it to become functional again all of a sudden and have my prioritisation strategy change and have strange behaviors in production.

WDYT?

@PatrickTulskie
Copy link
Contributor

@alxckn sorry I missed your response before... I think if we just fix it and put it in the release notes along with semver, it shouldn't be too surprising if this works again. Still seems like there might be a need for it.

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.

None yet

2 participants