Refactors nFlow codebase to use modern Java 17 functional patterns - #687
Refactors nFlow codebase to use modern Java 17 functional patterns#687ravening wants to merge 3 commits into
Conversation
| }); | ||
| int updatedRows = 0; | ||
| boolean unknownResults = false; | ||
| AtomicInteger updatedRows = new AtomicInteger(0); |
There was a problem hiding this comment.
Let's not add atomics to place where they are not needed. To export values out of a lambda we usually use
int[] updateRows = { 0 };and then just do
updatedRows[0] += updateStatus[i];
| } | ||
| } | ||
| return ids; | ||
| return Stream.iterate(0, i -> i < updateStatuses.length, i -> i + 1) |
There was a problem hiding this comment.
I would keep the old code here. Some code checkers say that there is no guarantee that peek is called on a stream. Maybe you could use mapMulti to convert the filter + map options to one?
Also the output list is no longer resized correctly.
| StringBuilder sb = new StringBuilder(2000); | ||
| for (StackTraceElement element : thread.getStackTrace()) { | ||
| sb.append(element).append('\n'); | ||
| String stack = stream(thread.getStackTrace()).map(Object::toString).collect(Collectors.joining("\n")); |
There was a problem hiding this comment.
Can you static import the joining
There was a problem hiding this comment.
Is it possible to pre-allocate the collector buffer to a large size? By default StringBuilder uses something like 16, which is way small for stack traces (and needs to be doubled many times). Have not checked what is the default for Collectors.joining though.
| for (StackTraceElement element : thread.getStackTrace()) { | ||
| sb.append(element).append('\n'); | ||
| String stack = stream(thread.getStackTrace()).map(Object::toString).collect(Collectors.joining("\n")); | ||
| StringBuilder sb = new StringBuilder(stack.length() + 2); |
There was a problem hiding this comment.
I don't think there is any point in wrapping the already created string into a StringBuilder again.
Just return String and do not append the final newline at all. Maybe the above logPotentiallyStuck looks ok without it. Or if you want to keep things the same, then just add an extra newline after the getStackTraceAsString parameter.
| public void handlePotentiallyStuck(Duration processingTime) { | ||
| boolean interrupt = false; | ||
| for (WorkflowExecutorListener listener : executorListeners) { | ||
| AtomicBoolean interrupt = new AtomicBoolean(false); |
There was a problem hiding this comment.
| AtomicBoolean interrupt = new AtomicBoolean(false); | |
| boolean[] interrupt = { false }; |
| DateTime currentTime = now(); | ||
| int potentiallyStuck = 0; | ||
| for (WorkflowStateProcessor processor : processingInstances.values()) { | ||
| AtomicInteger potentiallyStuck = new AtomicInteger(0); |
There was a problem hiding this comment.
| AtomicInteger potentiallyStuck = new AtomicInteger(0); | |
| int[] potentiallyStuck = { 0 }; |
| public WorkflowDefinitionStatisticsResponse convert(Map<String, Map<String, WorkflowDefinitionStatistics>> stats) { | ||
| WorkflowDefinitionStatisticsResponse resp = new WorkflowDefinitionStatisticsResponse(); | ||
| for (Entry<String, Map<String, WorkflowDefinitionStatistics>> entry : stats.entrySet()) { | ||
| stats.entrySet().forEach(entry -> { |
There was a problem hiding this comment.
Why not forEach directly on the map?
| action.executionStart, action.executionEnd, action.executorId, stateVariablesToJson(action.updatedStateVariables)) | ||
| : new Action(action.id, action.type.name(), action.state, action.stateText, action.retryNo, | ||
| action.executionStart, action.executionEnd, action.executorId)) | ||
| .collect(toList()); |
There was a problem hiding this comment.
Always prefer using the toList() method directly on the stream, not collector wrapper.
| .collect(toList()); | |
| .toList(); |
| Collection<State> values = states.values(); | ||
| resp.states = values.toArray(new State[values.size()]); | ||
| definition.getAllowedTransitions().forEach((key, targets) -> | ||
| Optional.ofNullable(states.get(key)).ifPresent(s -> s.transitions.addAll(targets))); |
There was a problem hiding this comment.
why the ofNullable? the original code assumed the state is consistent.
| if (def == null) { | ||
| throw new IllegalArgumentException("No workflow definition found for type [" + instance.type + "]"); | ||
| } | ||
| WorkflowDefinition def = Optional.ofNullable(workflowDefinitionService.getWorkflowDefinition(instance.type)) |
There was a problem hiding this comment.
I think the original code was more readable. And also the method continues doing null checks further down.
| value = ((Mutable<Object>) value).val; | ||
| if (value == null) { | ||
| continue; | ||
| Optional.ofNullable(value).ifPresent(v -> { |
There was a problem hiding this comment.
If you want to use Optional here, then use map method for the conversion and only the final setVariable should be in the ifPresent block.
2 participants
Summary
Refactors nFlow codebase to use modern Java 17 functional patterns:
Changes
Files Modified
Testing
✅ All 355+ tests passing in nflow-engine
✅ All 22+ tests passing in nflow-rest-api-common
✅ Full multi-module build verified
Notes