Entry points for recurring jobs #322
Conversation
…ne via the command line which j
src/test/java/org/opentripplanner/middleware/utils/CommandLineProcessorTest.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opentripplanner/middleware/utils/CommandLineProcessorTest.java
Outdated
Show resolved
Hide resolved
| if (command.equalsIgnoreCase("-recurringjobs")) { | ||
| defineRecurringJobs(commandValues); | ||
| } | ||
| if (command.equalsIgnoreCase("-loadbalance")) { |
There was a problem hiding this comment.
Trying to find a better name for this option without having to reference load balancers... How about something like --endpoints-only or --api-endpoints because this serves the API endpoints only, correct? (so that a load balancer can dispatch web requests).
There was a problem hiding this comment.
For command line flags, we should probably follow the convention used by many Unix programs - typically double-dash + kebab-case-command (e.g. --load-balancer) and either one dash + one letter/number (e.g. -L) for shorthand syntax. (Note: OTP uses pascalCase commands as in --loadBalancer and that's ok if we want to adopt that.)
There was a problem hiding this comment.
My thinking was to keep this generic, so anything that could be load balanced would be covered. At the moment, the only item is the API, so will change it to suit this.
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public enum RecurringJob { |
There was a problem hiding this comment.
This is probably okay as first approach. I had in mind that some recurring jobs should eventually be handled by the OS (e.g. cron) so that they can be started/terminated regardless of completion status.
src/main/java/org/opentripplanner/middleware/utils/ConfigUtils.java
Outdated
Show resolved
Hide resolved
| if (!recurringJobs.isEmpty()) { | ||
| recurringJobs.forEach(RecurringJob::schedule); | ||
| try { | ||
| // Block the main thread until the scheduler is terminated to prevent the application from |
There was a problem hiding this comment.
Maybe this is why I'd prefer recurring jobs to be scheduled by the OS itself?
There was a problem hiding this comment.
Yes, this isn't ideal. My approach for this first iteration was to not change the existing scheduled services. In the case of BugsnagJobs it actually schedules three different jobs with different parameters. Perhaps for a specific job (e.g. --cron-job) that qualifies, I can rearchitecture to have a Main entry point? We still need to consider backwards compatibility.
There was a problem hiding this comment.
It's fine to take it one step at a time.
| return this; | ||
| } | ||
|
|
||
| public CommandLineTestCase withLoadBalanced(boolean isLoadBalanced) { |
There was a problem hiding this comment.
Rename to withEndpoints (rename the parameter accordingly too).
| private static class CommandLineTestCase { | ||
| public String configFile = DEFAULT_ENV; | ||
| public Set<RecurringJob> recurringJobs = RecurringJob.getAllRecurringJobs(); | ||
| public boolean isLoadBalanced = true; |
There was a problem hiding this comment.
Rename to hasEndpoints.
| } | ||
| } | ||
|
|
||
| if (processor.hasEndPoints()) { |
There was a problem hiding this comment.
Move this block to before the scheduler stuff. Otherwise, because of the thread blocking above, this block is never executed (until termination), and therefore no endpoints are available.
| * Define if the end points should be enabled. | ||
| */ | ||
| private void defineEndPoints(Set<String> endPointArguments) { | ||
| if (endPointArguments.size() == 1 && endPointArguments.contains("yes")) { |
There was a problem hiding this comment.
In how the parsing code works, is it possible to just pass a --endpoint-only flag without the
"yes" or "no"? (so that --endpoints-only without a parameter implies "yes")
| * /configurations/test/env.yml --endpoints-only yes --recurring-jobs monitor-all-trips-job | ||
| */ | ||
| public class CommandLineProcessor { | ||
| public static final List<String> END_POINTS_ONLY_FLAGS = List.of("--endpoints-only", "-E"); |
There was a problem hiding this comment.
Sorry, rename to --endpoints because the flag is not meant to exclude the other features.
| if (RECURRING_JOB_FLAGS.contains(flag)) { | ||
| defineRecurringJobs(commandValues); | ||
| } |
There was a problem hiding this comment.
To only enable the endpoints and run no recurring jobs, the command line would read --recurring-jobs, and that feels kinda strange. Maybe including a --recurring-jobs no or --recurring-jobs none in addition to just --recurring-jobs would help reduce confusion.
|
|
||
| /** | ||
| * Parse the arguments provided on the command line. To maintain backward compatibility, if a config file is | ||
| * required, it must be the first argument and no command e.g. -config is NOT required. |
There was a problem hiding this comment.
| * required, it must be the first argument and no command e.g. -config is NOT required. | |
| * required, it must be the first argument, and no flag (e.g. --config) is used. |
| public static RecurringJob getJobFromCommandLineArgument(String commandLineArgument) { | ||
| return Arrays.stream(RecurringJob.values()) | ||
| .filter(job -> job.commandLineName.equalsIgnoreCase(commandLineArgument)) | ||
| .findFirst() | ||
| .orElse(null); | ||
| } |
There was a problem hiding this comment.
How does this method differ from the native Enum.valueOf()?
| } | ||
|
|
||
| public static Set<RecurringJob> getAllRecurringJobs() { | ||
| return new HashSet<>(Arrays.asList(RecurringJob.values())); |
There was a problem hiding this comment.
| return new HashSet<>(Arrays.asList(RecurringJob.values())); | |
| return Set.of(RecurringJob.values()); |
Checklist
devbefore they can be merged tomaster)Description
This PR seeks to allow fine grain control over the jobs within the OTP middleware which should be started. This will then allow various instances of OTP middleware to be started which focus on specific jobs instead of all jobs running under a single instance.
It should be backwards compatiable, e.g. defining no parameters or defining just a config file should allow everything to start under the one instance.