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

feature request: make executor rescheduling configurable - no lambda implementation proposal #4188

Open
wants to merge 2 commits into
base: release/4.2.x
Choose a base branch
from

Conversation

github-louis-fruleux
Copy link

@github-louis-fruleux github-louis-fruleux commented May 23, 2024

Hello 👋
First of all, thanks for maintaining the project, we appreciate it.

We had an issue when using Grafana, actually, the exact same as described here: #2664

While having the possibility to override it is nice, we lose a lot of things by doing so, including the nice API builder on the graphite report.
The PR aims to make it configurable with the constructor.

Would that make sense to move forward with this PR?
Feel free to make any comments, long time I have not done any Java. I might not be aware of the good practices

PS: I made another PR proposition here, #4192. Which does not use lambdas and is maybe cleaner to read / maintain. What do you think?

@github-louis-fruleux github-louis-fruleux marked this pull request as ready for review May 23, 2024 12:30
@github-louis-fruleux github-louis-fruleux requested review from a team as code owners May 23, 2024 12:30
@github-louis-fruleux github-louis-fruleux changed the title make executor rescheduling configurable feature request: make executor rescheduling configurable May 23, 2024
Comment on lines -174 to -183
/**
* Schedule the task, and return a future.
*
* @deprecated Use {@link #getScheduledFuture(long, long, TimeUnit, Runnable, ScheduledExecutorService)} instead.
*/
@SuppressWarnings("DeprecatedIsStillUsed")
@Deprecated
protected ScheduledFuture<?> getScheduledFuture(long initialDelay, long period, TimeUnit unit, Runnable runnable) {
return getScheduledFuture(initialDelay, period, unit, runnable, this.executor);
}

Choose a reason for hiding this comment

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

I am aware this is a breaking change. Unfortunately, I did not find a way to make this function still relevant while passing the functional interface
Additionally, I think it is unlikely someone is overriding this method because it can make sense only with an executor.

What do you think?
Something obvious I missed in keeping this method for retro compatibility, while still making it relevant?

@joschi joschi force-pushed the make-executor-rescheduling-configurable branch from 6714710 to 3dd5b7a Compare October 3, 2024 17:00
@github-louis-fruleux github-louis-fruleux changed the title feature request: make executor rescheduling configurable feature request: make executor rescheduling configurable - no lambda implementation proposal Oct 4, 2024
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.

1 participant