Skip to content

Add endpoint for scheduled task information #9623

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

Closed
wants to merge 2 commits into from
Closed

Add endpoint for scheduled task information #9623

wants to merge 2 commits into from

Conversation

htynkn
Copy link
Contributor

@htynkn htynkn commented Jun 28, 2017

Some of our team projects are using many scheduled task, so we custom one endpoint to list four different type of scheduled tasks.

Not sure if there is what issue #8831 is looking for, but at least I think it's a good start if any one want to add more information in this endpoint.

This is a example response:
screen shot 2017-06-28 at 9 01 21 am

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 28, 2017
@philwebb philwebb added priority: normal type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 28, 2017
@philwebb philwebb added this to the 2.0.0.M4 milestone Jun 28, 2017
@philwebb
Copy link
Member

This looks super nice. Thanks.

@wilkinsona
Copy link
Member

Thank you, @htynkn.

Per #7648, when it becomes time to merge this, we should change the format of the response so that it returns a map rather than a list.

@htynkn No need to make that change now. I just wanted to make a note of it while I remembered. The Actuator is going to be reworked to support WebFlux in M3 so there may be other adjustments required once that's been done.

@snicoll
Copy link
Member

snicoll commented Jun 28, 2017

Also, we don't have an explicit support for @EnableScheduling in Spring Boot but we do for Quartz so we need to figure out how to handle that situation.

@philwebb philwebb added the for: merge-with-amendments Needs some changes when we merge label Jul 6, 2017
public class ScheduledTaskInformation {
private ScheduledType type;
private long interval;
private long initialDelay;
Copy link

Choose a reason for hiding this comment

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

If you switch interval and initialDelay to java.lang.Long, then you can avoid unnecessary zeroes in the response for CRON tasks.

Before:

[
    {
        "Type": "CRON",
        "Interval": 0,
        "InitialDelay": 0,
        "Name": "public void com.foo.Bar.update()",
        "Expression": "0 */5 * * * *",
        "Trigger": "CronSequenceGenerator: 0 */5 * * * *"
    }
]

After:

[
    {
        "Type": "CRON",
        "Name": "public void com.foo.Bar.update()",
        "Expression": "0 */5 * * * *",
        "Trigger": "CronSequenceGenerator: 0 */5 * * * *"
    }
]

@wilkinsona
Copy link
Member

Thanks for the suggestion, @WispY. I think this will be taken care of when we rework the format of the response to return a map rather than an array. I imagine that we'll use the task types as keys and arrays of tasks of a particular type as the values.

}

/**
* Create a new {@link ScheduledTaskEndpoint} instance.

Choose a reason for hiding this comment

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

Looks like this comment is placed on the wrong method and should be on constructor.

}

private List<ScheduledTaskInformation> build(final List<? extends Task> tasks, final ScheduledType type) {
return tasks.stream().map(task -> {

Choose a reason for hiding this comment

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

With so long method, usage of stream and map looks like of abusing such functionality IMHO.

if (task instanceof TriggerTask) {
TriggerTask triggerTask = (TriggerTask) task;
scheduledTaskInformation.setTrigger(triggerTask.getTrigger().toString());
}
Copy link

@php-coder php-coder Jul 13, 2017

Choose a reason for hiding this comment

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

I suspect that it's impossible that the task is the IntervalTask and CronTask and TriggerTask at the same time. In this case, it's better to use if-else if-else if.

}

/**
* Four different scheduled task type.

Choose a reason for hiding this comment

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

This comment has details that is obvious and that can be changed in a future and will make this comment outdated. Also as a user who will read the API docs I don't need to know how many types here, because I can count them by myself.

assertThat(cronTaskOptional.get().getName()).contains("Config.cronMethod");
}

private Optional<ScheduledTaskEndpoint.ScheduledTaskInformation> filterTaskInformation(

Choose a reason for hiding this comment

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

Returning Optional here didn't make the code simpler IMHO. For me, reading something like the following would be more readable

assertThat(fixedRateTask).isNotNull();
assertThat(fixedRateTask.getInterval()).isEqualTo(2000);
assertThat(fixedRateTask.getInitialDelay()).isEqualTo(0);

than we we have now:

assertThat(fixedRateTaskOptional.isPresent()).isTrue();
assertThat(fixedRateTaskOptional.get().getInterval()).isEqualTo(2000);
assertThat(fixedRateTaskOptional.get().getInitialDelay()).isEqualTo(0);

if (runnable instanceof ScheduledMethodRunnable) {
return ((ScheduledMethodRunnable) runnable).getMethod().toGenericString();
}
return runnable.toString();

Choose a reason for hiding this comment

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

NPE will be thrown here if runnable is null.

@wilkinsona wilkinsona modified the milestones: 2.0.0.M5, 2.0.0.M4 Jul 28, 2017
@wilkinsona wilkinsona self-assigned this Sep 20, 2017
@wilkinsona
Copy link
Member

Having looked a little more closely, I'm not keen on how the configurer is being used here. In addition to the callback not really being used as intended, there's also an ordering issue that means tasks may be missed. I've opened SPR-15982 to see if Spring Framework can provide a supported API for retrieving the registered tasks. I'll place this on hold until we see the outcome of that Framework issue.

@wilkinsona wilkinsona added status: on-hold We can't start working on this issue yet and removed for: merge-with-amendments Needs some changes when we merge labels Sep 20, 2017
@wilkinsona
Copy link
Member

The proposed Framework changes are now targeted at 5.0.1. Pushing back to M6.

@wilkinsona wilkinsona removed this from the 2.0.0.M5 milestone Sep 24, 2017
@wilkinsona
Copy link
Member

The proposed Framework changes are now targeted at 5.0.2. Pushing back to RC1.

@wilkinsona wilkinsona modified the milestones: 2.0.0.M6, 2.0.0.RC1 Oct 25, 2017
@wilkinsona
Copy link
Member

Thanks for the PR. Unfortunately, with the changes to the endpoint infrastructure and the improvements coming in Spring Framework 5.0.2 for accessing scheduled tasks, this isn't the approach that we want to take to implement this. Thanks anyway.

@wilkinsona wilkinsona closed this Nov 14, 2017
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: on-hold We can't start working on this issue yet labels Nov 14, 2017
@wilkinsona wilkinsona removed this from the 2.0.0.M7 milestone Nov 14, 2017
@snicoll snicoll removed priority: normal type: enhancement A general enhancement labels Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants