server: add a 30 minute timeout for event trigger locks #5473

paf31 · 2020-07-28T20:49:30Z

This gives event trigger locks a 30 minute timeout. In case the graceful shutdown of a server fails, this will automatically unlock in-process rows after 30 minutes at most.

This PR is a work in progress and I haven't had chance to test it extensively yet, but we can use this as a place to discuss the approach.

Changelog

CHANGELOG.md is updated with user-facing content relevant to this PR. If no changelog is required, then add the no-changelog-required label.

Affected components

Server

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

No
Yes
- Updated docs with SQL for downgrading the catalog

Metadata

Does this PR add a new Metadata feature?

No

GraphQL

No new GraphQL schema is generated

Breaking changes

There are breaking changes:

This changes the type of the locked column to be a timestamp null instead of a boolean not null.

hasura-bot · 2020-07-28T21:05:04Z

Review app for commit fe70d9f deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-fe70d9fb

jberryman

LGTM

server/src-lib/Hasura/Eventing/EventTrigger.hs

server/src-lib/Hasura/RQL/DDL/EventTrigger.hs

server/src-rsr/initialise.sql

Co-authored-by: Brandon Simmons <brandon.m.simmons@gmail.com>

hasura-bot · 2020-07-28T23:27:53Z

Review app for commit ca190ef deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-ca190ef5

tirumaraiselvan · 2020-07-29T04:10:03Z

@paf31 IIUC, we can now get rid of this?

graphql-engine/server/src-lib/Hasura/App.hs

Line 371 in df51a8e

prepareEvents _icPgPool logger

codingkarthik · 2020-07-29T05:43:34Z

server/src-rsr/initialise.sql

  created_at TIMESTAMP DEFAULT NOW(),
-  locked BOOLEAN NOT NULL DEFAULT FALSE,
+  /* when locked IS NULL the event is unlocked and can be processed */
+  locked TIMESTAMP,


Not sure if this will affect the behaviour, but I think it will be a good idea to use TIMESTAMPZ instead of TIMESTAMP because now() returns a TIMESTAMPZ and there may be issues when comparing a TIMESTAMPZ value with a TIMESTAMP value

paf31 · 2020-07-29T20:27:00Z

@paf31 IIUC, we can now get rid of this?

graphql-engine/server/src-lib/Hasura/App.hs

Line 371 in df51a8e

prepareEvents _icPgPool logger

That code unlocks all events if I understand correctly. I think it might be good to keep it, so that we have a way to quickly unlock everything without having to wait 30 minutes.

hasura-bot · 2020-07-29T20:43:22Z

Review app for commit c93a00b deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-c93a00b3

hasura-bot · 2020-07-30T01:26:19Z

Review app for commit 8cc548a deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-8cc548ab

hasura-bot · 2020-07-30T02:52:17Z

Review app for commit ab73923 deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-ab73923a

tirumaraiselvan · 2020-07-30T03:37:46Z

That code unlocks all events if I understand correctly. I think it might be good to keep it, so that we have a way to quickly unlock everything without having to wait 30 minutes.

We are tracking events which are in-memory and unlocking them during graceful shutdown, right?

graphql-engine/server/src-lib/Hasura/App.hs

Line 462 in 2c397f9

shutdownEvents

This way a proper server restart should not wait 30 mins to retry a locked event.

Shouldn't this cover most cases? Otherwise, I am not really sure what is the ROI of changing the locked column from bool to timestamp. We are managing the same amount of state as before?

jberryman · 2020-07-30T15:15:57Z

Mm, yeah I agree we shouldn't be doing unlockAllEvents on startup anymore. Formerly I guess that was the only way to ensure at-least-once delivery in the case of a hard shutdown. But now that case is taken care of by having the timeout state in durable storage.

While this is still open, do we want to think more about the timeout for (re)delivery? Maybe it should be much shorter or be relative to the http client timeout value we use? Just a detail.

hasura-bot · 2020-07-30T17:18:01Z

Review app for commit 5dee537 deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-5dee5373

paf31 · 2020-07-30T19:33:26Z

While this is still open, do we want to think more about the timeout for (re)delivery?

Definitely. I wasn't sure about this so I just did the most naive thing possible to start with. Any ideas?

…-engine into event-trigger-lock-timeout

jberryman · 2020-07-30T22:42:33Z

Any ideas?

I'm not sure, and setting it too low is the real danger (sort of slow thundering herd situation... lumbering herd). Seems good for now

hasura-bot · 2020-08-10T19:54:43Z

Review app for commit b3856d8 deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-b3856d82

jberryman · 2020-08-10T20:15:58Z

(sorry if this was awaiting another approval by me)

paf31 · 2020-08-12T02:03:59Z

(sorry if this was awaiting another approval by me)

Sorry, it may have, but I'm also being a little hesitant to merge this, since it is a little difficult to test and be sure that it is the best change, for arguably little benefit. Since the original problem is (I think) now addressed, I'm thinking we can hold off until I can write some tests for this. What do you think?

hasura-bot · 2020-08-12T02:17:19Z

Review app for commit 4cc7394 deployed to Heroku: https://hge-ci-pull-5473.herokuapp.com
Docker image for server: hasura/graphql-engine:pull5473-4cc73949

tirumaraiselvan · 2020-08-12T09:17:43Z

I have marked this for v1.4 release since we get a beta period to test this change.

codingkarthik

LGTM

jberryman · 2020-09-09T17:10:49Z

(FWIW if you want to try using kodiak for auto-merge/update you can add the auto-update-auto-merge tag)

Add a 30 minute timeout for event trigger locks

fe70d9f

paf31 added the no-changelog-required label Jul 28, 2020

paf31 requested review from jberryman and tirumaraiselvan July 28, 2020 20:49

jberryman approved these changes Jul 28, 2020

View reviewed changes

server/src-lib/Hasura/Eventing/EventTrigger.hs Outdated Show resolved Hide resolved

server/src-lib/Hasura/RQL/DDL/EventTrigger.hs Outdated Show resolved Hide resolved

server/src-rsr/initialise.sql Outdated Show resolved Hide resolved

paf31 and others added 2 commits July 28, 2020 16:11

Apply suggestions from code review

02ddcb3

Co-authored-by: Brandon Simmons <brandon.m.simmons@gmail.com>

Merge branch 'master' into event-trigger-lock-timeout

ca190ef

codingkarthik reviewed Jul 29, 2020

View reviewed changes

Merge branch 'master' into event-trigger-lock-timeout

c93a00b

Use TIMESTAMPTZ

8cc548a

paf31 marked this pull request as ready for review July 29, 2020 21:14

paf31 requested a review from 0x777 July 29, 2020 21:14

Merge branch 'master' into event-trigger-lock-timeout

ab73923

Merge branch 'master' into event-trigger-lock-timeout

5dee537

paf31 and others added 3 commits July 30, 2020 12:33

Remove prepareEvents

b24af62

Merge branch 'event-trigger-lock-timeout' of github.com:paf31/graphql…

c204f40

…-engine into event-trigger-lock-timeout

Merge branch 'master' into event-trigger-lock-timeout

253536c

jberryman approved these changes Jul 30, 2020

View reviewed changes

tirumaraiselvan added this to the v1.4 milestone Aug 6, 2020

paf31 added 2 commits August 10, 2020 09:54

Merge branch 'master' into event-trigger-lock-timeout

bdfd831

Merge branch 'master' into event-trigger-lock-timeout

b3856d8

jberryman approved these changes Aug 10, 2020

View reviewed changes

Merge branch 'master' into event-trigger-lock-timeout

4cc7394

codingkarthik approved these changes Sep 9, 2020

View reviewed changes

Merge branch 'master' into event-trigger-lock-timeout

ff5786e

paf31 added the auto-update-auto-merge label Sep 9, 2020

kodiakhq bot merged commit e680f2c into hasura:master Sep 9, 2020

codingkarthik mentioned this pull request Oct 15, 2020

Events stuck in locked state, never attempted. #6025

Open

server: add a 30 minute timeout for event trigger locks #5473

server: add a 30 minute timeout for event trigger locks #5473

Uh oh!

Conversation

paf31 commented Jul 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Metadata

GraphQL

Breaking changes

Uh oh!

hasura-bot commented Jul 28, 2020

Uh oh!

jberryman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hasura-bot commented Jul 28, 2020

Uh oh!

tirumaraiselvan commented Jul 29, 2020

Uh oh!

codingkarthik Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

paf31 commented Jul 29, 2020

Uh oh!

hasura-bot commented Jul 29, 2020

Uh oh!

hasura-bot commented Jul 30, 2020

Uh oh!

hasura-bot commented Jul 30, 2020

Uh oh!

tirumaraiselvan commented Jul 30, 2020

Uh oh!

jberryman commented Jul 30, 2020

Uh oh!

hasura-bot commented Jul 30, 2020

Uh oh!

paf31 commented Jul 30, 2020

Uh oh!

jberryman commented Jul 30, 2020

Uh oh!

hasura-bot commented Aug 10, 2020

Uh oh!

jberryman commented Aug 10, 2020

Uh oh!

paf31 commented Aug 12, 2020

Uh oh!

hasura-bot commented Aug 12, 2020

Uh oh!

tirumaraiselvan commented Aug 12, 2020

Uh oh!

codingkarthik left a comment

Choose a reason for hiding this comment

Uh oh!

jberryman commented Sep 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

paf31 commented Jul 28, 2020 •

edited

Loading