Review of Jakarta Concurrency guide #70
Description
"Jakarta Concurrency enables the creation of managed executors to run tasks in separate threads with access to server contexts."
change to:
"Jakarta Concurrency enables the creation of managed executors to run tasks in separate threads with access to the context of the submitter."
This sentence is awkward:
"The threads are managed by the application server, coexist, access server contexts, and safely interact in the Enterprise Java Beans and Web containers."
change to:
"The threads are managed by the application server and safely interact in the Enterprise Java Beans and Web containers."
"The application that you’ll build provides REST APIs to retrieve system properties and record system loads including CPU load and memory usage to the database."
change to:
"The application that you will build provides REST APIs to retrieve system properties and write information about system loads, including CPU load and memory usage, to the database."
A general comment is that the use of "you'll" all over the guide looks very unprofessional.
"Before you begin, make sure you have all the necessary prerequisites."
You should state what the prerequisites are so the reader isn't left wondering where to find out what they are.
grammar error in:
"The starting application is implemented the /properties/{prefix} endpoint."
The link to the class is broken in,
"Enable the SystemProperties class "
"Enable the SystemProperties class to use Jakarta Concurrency managed executor service that provides a way to"
change to:
"Update the SystemProperties class to use a Jakarta Concurrency managed executor service to"
"Replace the SystemProperties.java file."
Copying in a new file on top of another doesn't help the user learn anything. Individual code blocks should be copied over step by step along with explanation of why you are doing each. For example,
To define a managed executor service, ...
To inject an instance of the managed executor service into your application, ...
Use the managed executor service to ...
"The SystemProperties class is annotated by the @ManagedExecutorDefinition annotation to provide a managed executor instance through the ManagedExecutorService injection point."
You could do that, but what you have coded doesn't. You coded up a ManagedExecutorDefinition that defines an instance with a particular name and no qualifiers. Your injection point of @Inject ManagedExecutorService managedExecutor; specifies no qualifiers and so it gets the default instance of managed executor service, not the one you configured.
Your example code for getProperties(String prefix) is difficult to follow, which obscures its use of the managed executor service. Someone might write it more concisely and understandably like this:
ConcurrentHashMap<String, String> properties = new ConcurrentHashMap<>();
List<Callable<String>> tasks = new ArrayList<>();
for (String key : System.getProperties().stringPropertyNames())
if (key.startsWith(prefix)) {
tasks.add(() -> {
return properties.put(key, getSystemPropertyTask(key));
});
}
managedExecutor.invokeAll(tasks);
return properties;
SystemConcurrency.java has the same problem as the other class, with
@Inject
ManagedScheduledExecutorService managedExecutor;
This injects the default instance rather than the one defined by @ManagedScheduledExecutorDefinition.
ut.commit();
logger.info("CPU load at \"" + current + "\" was recorded.");
} catch (Exception e) {
logger.warning(e.getMessage());
}
Well-written code will always ensure to roll back the transaction in case of failure. You should correct that here so that users don't copy a bad practice.
"annotation to make it running asynchronously"
change to:
"annotation to make it run asynchronously"
"mange the jpa-unit"
change to:
"manage the jpa-unit"
Neither persistence context nor transaction context is being propagated here, so this is wrong:
"the jpa-unit persistence context that is propagated with transaction context in the threads to ensure that data integrity is preserved."
Change to:
"the application context in order to make the jpa-unit persistence unit available to the asynchronous task."
"Create the SystemConcurrency.java file."
You should have this file pre-created and the user should be guided to copy it the relevant parts.
"Replace the SystemResource.java file."
I don't see the point of this. There is nothing in this file to teach the user about Jakarta Concurrency. Just provide the file in advance.
"Scheduling asynchronous task"
Change to:
"Scheduling an asynchronous method"
There is a different way in the spec to schedule asynchronous tasks that are not methods.
"by using scheduled asynchronous method."
change to:
"by using a scheduled asynchronous method."
"Replace the SystemConcurrency.java file."
Again, avoid replacing files. The user doesn't learn anything that way. Have them paste in smaller code block as you explain what they are doing.
The names of these methods are confusing because they don't actually switch the scheduled method on and off. What you are really doing is indicating when the scheduled method should stop running.
public boolean isScheduleEnabled() {
return scheduleEnabled;
}
public void enableSchedule(boolean enabled) {
scheduleEnabled = enabled;
}
You should replace it with a method called something like stopRepeating() or discontinueSchedule().
"cron expression to make the task running asynchronously in every 10 seconds"
change to:
"cron expression to make the task run asynchronously every 10 seconds (after the seconds of each minute that are divisible by 10)"
You should really link to the CRON syntax here for users to read about if they wish to understand it.
https://jakarta.ee/specifications/concurrency/3.1/apidocs/jakarta.concurrency/jakarta/enterprise/concurrent/crontrigger
"The added GET /schedule/toggle endpoint"
Don't have the user add this. It is just a Jakarta REST method, not anything related to Jakarta Concurrency.