[GitHub] [nifi] turcsanyip commented on a change in pull request #4883: NIFI-8289 Refinement on embedded QuestDB based status repository rollover mechanism: adding time zone support; preventing active partition from removal

[GitBox <gi...@apache.org>]

turcsanyip commented on a change in pull request #4883:
URL: https://github.com/apache/nifi/pull/4883#discussion_r591942886



##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/EmbeddedQuestDbRolloverHandler.java
##########
@@ -48,16 +47,22 @@
     // Distinct keyword is not recognized if the date mapping is not within an inner query
     static final String SELECTION_QUERY = "SELECT DISTINCT * FROM (SELECT (to_str(capturedAt, 'yyyy-MM-dd')) AS partitionName FROM %s)";
 
-    static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd").withZone(ZoneId.systemDefault());
+    DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd").withZone(ZoneId.of("UTC"));

Review comment:
       `ZoneOffset.UTC` constant could be used.

##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/EmbeddedQuestDbRolloverHandler.java
##########
@@ -48,16 +47,22 @@
     // Distinct keyword is not recognized if the date mapping is not within an inner query
     static final String SELECTION_QUERY = "SELECT DISTINCT * FROM (SELECT (to_str(capturedAt, 'yyyy-MM-dd')) AS partitionName FROM %s)";
 
-    static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd").withZone(ZoneId.systemDefault());
+    DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("yyyy-MM-dd").withZone(ZoneId.of("UTC"));

Review comment:
       It can remain `static final`.

##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/test/java/org/apache/nifi/controller/status/history/EmbeddedQuestDbRolloverHandlerTest.java
##########
@@ -149,43 +247,39 @@ private void givenTableIsCreated(final QuestDbContext dbContext) throws Exceptio
         dbContext.getCompiler().compile(CREATE_TABLE, dbContext.getSqlExecutionContext());
     }
 
-    private void givenTableIsPopulated(final List<Long> givenMeasurementTimes) {
+    private void givenTableIsPopulated(final String... dates) throws Exception {
+        int value = 0;
+
+        for (final String date : dates) {
+            givenTableIsPopulated(date, value++);
+        }
+    }
+
+    private void givenTableIsPopulated(final String date, final int value) throws Exception {
+        final DateTimeFormatter formatter = DateTimeFormatter.ofPattern("MM/dd/yyyy HH:mm:ss z").withZone(ZoneId.systemDefault());

Review comment:
       As the format has zone part (`z`), I don't think `withZone()` could have any effect.
   Also in line 274.

##########
File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/status/history/EmbeddedQuestDbRolloverHandler.java
##########
@@ -105,19 +112,13 @@ private void deletePartition(final CharSequence tableName, final String partitio
             }
         }
 
+        Collections.sort(result);
         return result;
     }
 
-    private Set<String> getPartitionsToKeep() {
-        final Instant now = Instant.now();
-
-        // Note: as only full partitions might be deleted and the status history repository works with day based partitions,
-        // a partition must remain until any part of it might be the subject of request.
-        final Set<String> result = new HashSet<>();
-        for (int i = 0; i < daysToKeepData + 1; i++) {
-            result.add(DATE_FORMATTER.format(now.minus(i, ChronoUnit.DAYS)));
-        }
-
-        return result;
+    private String getOldestPartitionToKeep() {
+        final ZonedDateTime now = timeSource.get();
+        final ZonedDateTime utc = now.minusDays(daysToKeepData).withZoneSameInstant(ZoneId.of("UTC"));

Review comment:
       `ZoneOffset.UTC` constant could be used.




----------------------------------------------------------------
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