[GitHub] [incubator-superset] villebro commented on a change in pull request #11890: fix(reports): validator_config, report state machine, working_timeout

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

villebro commented on a change in pull request #11890:
URL: https://github.com/apache/incubator-superset/pull/11890#discussion_r536032049



##########
File path: superset/models/reports.py
##########
@@ -122,7 +123,10 @@ class ReportSchedule(Model, AuditMixinNullable):
 
     # Log retention
     log_retention = Column(Integer, default=90)
+    # (Alerts) After a success how long to wait for a new trigger (seconds)
     grace_period = Column(Integer, default=60 * 60 * 4)
+    # (Alerts/Reports) Unlock a possible stalled working state
+    working_timeout = Column(Integer, default=60 * 60 * 1)

Review comment:
       Should defaults be defined in the application rather than in the metadata? It feels clearer if we leave defaults as `null` to make it possible to change defaults later.

##########
File path: superset/reports/commands/alert.py
##########
@@ -70,7 +76,7 @@ def _validate_operator(self, rows: np.recarray) -> None:
             raise AlertQueryMultipleColumnsError(
                 _(
                     "Alert query returned more then one column. %s columns returned"
-                    % len(rows[0])
+                    % (len(rows[0]) - 1)

Review comment:
       A comment here could be helpful (I assume this is the case where the first column in the index).

##########
File path: superset/reports/commands/execute.py
##########
@@ -192,45 +201,177 @@ def _send(self, report_schedule: ReportSchedule) -> None:
         if notification_errors:
             raise ReportScheduleNotificationError(";".join(notification_errors))
 
+    def is_in_grace_period(self) -> bool:
+        """
+        Checks if an alert is on it's grace period
+        """
+        last_success = ReportScheduleDAO.find_last_success_log(
+            self._report_schedule, session=self._session
+        )
+        return (
+            last_success is not None
+            and self._report_schedule.grace_period
+            and datetime.utcnow()
+            - timedelta(seconds=self._report_schedule.grace_period)
+            < last_success.end_dttm
+        )
+
+    def is_working_timeout(self) -> bool:

Review comment:
       Should this be called `is_on_working_timeout` just to make it clear that it has exceeded the threshold? To someone with less knowledge `is_working_timeout` might imply that a timeout is working.




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



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org