Timeouts in session closing and closed reminders #5999 #6001
Conversation
| Query q = getPm().newQuery(FeedbackSession.class); | ||
| q.declareParameters("boolean sentParam, boolean enableParam, Enum notTypeParam"); | ||
| q.setFilter("sentClosedEmail == sentParam && isClosedEmailEnabled == enableParam " | ||
| + "&& feedbackSessionType != notTypeParam"); |
There was a problem hiding this comment.
We can't filter by closing time?
There was a problem hiding this comment.
Not that trivial. A lot of matters need to be considered, one of which is timezone.
There was a problem hiding this comment.
Oh right, forgot about that when I was proposing solution 1 😛
There was a problem hiding this comment.
We can ignore the timezone. Just get the sessions having closing times within last 48 hours. This should bring down the result set to less than a hundred instead of 13k.
There was a problem hiding this comment.
Adding a new a attribute is a lot more complicated as it needs to be kept consistent when the session deadlines are changed. Furthermore, there is a risk that we'll end up sending the reminder repeatedly if something goes wrong with setting the flag. I don't mind doing it if it can be done in a fool proof way, but I think the quickest solution is to narrow down the result set into last 48 hours.
There was a problem hiding this comment.
It's already done the foolproof way. If not, then something is also wrong with how we manage the opening and published emails.
|
Looks okay now 👍 |
chowyb
added
s.FinalReview
damithc
added
s.ToMerge
|
Can merge. Should have pinged me earlier. |
|
Released as V5.87.01. |
|
What's the behavior for legacy data? e.g. during the brief period of data migration |
|
Also, should I deploy first and then run script or the other way around? |
|
Another thing: shouldn't we migrate all sessions, not just non-private ones? I think private ones can become non-private if the user changes the flag. |
|
Waiting for some clarification before deployment. |
|
Migrating in progress. Sometimes migration scripts timeout in the middle due to server dropping the connection. That's why migration scripts should be able to run repeatedly and should be able to skip the entities already migrated. |
|
Noted. Apologies for that, this was only my second time writing client script (the first one being the course timezone field). |
|
We'll probably need to document client script guidelines somewhere. At the moment they are just passed down by word-of-mouth. |
Fixes #5999
Still running some tests (in particular for checking backward-compatibility), but feel free to review what's already here.