nifi-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [nifi] tpalfy commented on a change in pull request #4420: NIFI-7429 Adding status history for system level metrics
Date Fri, 04 Sep 2020 12:59:03 GMT

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:
users@infra.apache.org



Mime
View raw message