tpalfy commented on a change in pull request #4420:
URL: https://github.com/apache/nifi/pull/4420#discussion_r483599157
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/StatusHistoryUtil.java
##########
@@ -76,14 +76,14 @@ public static StatusDescriptorDTO
createStatusDescriptorDto(final MetricDescript
public static List<StatusDescriptorDTO> createFieldDescriptorDtos(final
Collection<MetricDescriptor<?>> metricDescriptors) {
final List<StatusDescriptorDTO> dtos = new ArrayList<>();
+ final Map<Integer, MetricDescriptor<?>> orderedDescriptors = new
HashMap<>();
- final Set<MetricDescriptor<?>> allDescriptors = new LinkedHashSet<>();
for (final MetricDescriptor<?> metricDescriptor : metricDescriptors) {
Review comment:
I think I didn't explain myself clearly. I understand the reasoning and
constraints but those don't deny my statements.
My code snippet was not 100% correct but the concept was. Here's a correct
version:
```java
final StatusDescriptorDTO[] dtos = new
StatusDescriptorDTO[metricDescriptors.size()];
for (final MetricDescriptor<?> metricDescriptor : metricDescriptors)
{
dtos[metricDescriptor.getMetricIdentifier()] =
createStatusDescriptorDto(metricDescriptor);
}
return Arrays.asList(dtos);
```
Here's a unit test (which - or something similar - would be useful to add):
```java
public class StatusHistoryUtilTest {
@Test
public void name() throws Exception {
// GIVEN
Collection<MetricDescriptor<?>> metricDescriptors = Arrays.asList(
new StandardMetricDescriptor<>(
() -> 1,
"field2",
"Field2",
"Field 2",
MetricDescriptor.Formatter.COUNT,
__ -> 2L
),
new StandardMetricDescriptor<>(
() -> 0,
"field1",
"Field1",
"Field 1",
MetricDescriptor.Formatter.COUNT,
__ -> 1L
)
);
List<StatusDescriptorDTO> expected = Arrays.asList(
new StatusDescriptorDTO("field1", "Field1", "Field 1",
MetricDescriptor.Formatter.COUNT.name()),
new StatusDescriptorDTO("field2", "Field2", "Field 2",
MetricDescriptor.Formatter.COUNT.name())
);
// WHEN
List<StatusDescriptorDTO> actual =
StatusHistoryUtil.createFieldDescriptorDtos(metricDescriptors);
// THEN
assertEquals(expected, actual);
}
}
```
There are two metric indexes: 1, 0 (coming from `() -> 1` and `() -> 0,`
respectively).
- If you change `() -> 1` to `() -> 2`, the method throws an expection
(yours a NullPointerException, mine an ArrayIndexOutOfBoundsException) -
Because index 1 is missing. That's what I meant by "metricDescriptors cannot
have a gap in their getMetricIdentifier() values"
- All we do is make sure the order of the output is based on the metric
index. That can done with the simple for iteration I presented, or simply
sorting it, mapping them into a dto and collecting them into a list (which
_would_ allow a gap in the getMetricIdentifier() values as well) like this:
```java
public static List<StatusDescriptorDTO> createFieldDescriptorDtos(final
Collection<MetricDescriptor<?>> metricDescriptors) {
return metricDescriptors.stream()
.sorted((a, b) -> a.getMetricIdentifier() <
b.getMetricIdentifier() ? -1 : a.getMetricIdentifier() >
b.getMetricIdentifier() ? 1 :0)
.map(StatusHistoryUtil::createStatusDescriptorDto)
.collect(Collectors.toList());
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
