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:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to