WIP: PostgreSQL LISTEN/NOTIFY: invalidate wit cache #2172

kwk · 2018-07-17T03:30:42Z

PLEASE, DO NOT MERGE AS THIS MIGHT NEED APPROVAL FROM A POTENTIALLY LARGE AUDIENCE!!!

This change uses PostgreSQL's LISTEN/NOTIFY concept as an inter-process communication to signal that all core pods shall reload their work item cache after a space template was updated.

Here's the piece of code that subscribes to a channel:

// Ensure we delete the work item cache when we receive a notification from postgres
gormsupport.SetupDatabaseListener(*config, map[string]gormsupport.SubscriberFunc{
	gormsupport.ChanSpaceTemplateUpdates: func(channel, extra string) {
		workitem.ClearGlobalWorkItemTypeCache()
	},
})

This concept can be reused very easily by adding new listeners for the individual event channels.

TODO:

more negative tests

codecov-io · 2018-07-17T03:46:01Z

Codecov Report

Merging #2172 into master will decrease coverage by 0.03%.
The diff coverage is 46.93%.

@@            Coverage Diff             @@
##           master    #2172      +/-   ##
==========================================
- Coverage   69.59%   69.56%   -0.04%     
==========================================
  Files         170      171       +1     
  Lines       15681    15730      +49     
==========================================
+ Hits        10913    10942      +29     
- Misses       3774     3794      +20     
  Partials      994      994

Impacted Files	Coverage Δ
migration/migration.go	`59.28% <0%> (-1.39%)`	⬇️
configuration/configuration.go	`69.45% <100%> (+0.53%)`	⬆️
gormsupport/listener.go	`47.22% <47.22%> (ø)`
controller/workitem.go	`79.84% <0%> (+1.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b6c848...68f445a. Read the comment docs.

kwk · 2018-07-17T03:56:40Z

gormsupport/listener_test.go

+
+		// Send a notification from a completely different connection than the
+		// one we established to listen to channels.
+		s.DB.Debug().Exec("SELECT pg_notify($1, $2)", channelName, payload)


Mental note for myself: I need to check for db.Error here.

Done in b227b9f

kwk · 2018-07-17T03:57:04Z

gormsupport/listener_test.go

+
+		// This will send a notification on the
+		// gormsupport.ChanSpaceTemplateUpdates channel
+		migration.PopulateCommonTypes(nil, s.DB)


Mental note for myself: I need to check for err here.

Done in b227b9f

kwk · 2018-07-17T04:14:54Z

[test]

jarifibrahim · 2018-07-17T06:35:09Z

@kwk I think this would fix openshiftio/openshift.io#3930.

baijum · 2018-07-17T07:08:22Z

Why don't we use a cache server like Memcached?

kwk · 2018-07-17T11:03:35Z

Why don't we use a cache server like Memcached?

@baijum I think the effort to maintain a memcached server and have it deployed is outweighed by the simplicity and tight connection we already have with postgres. Also note that we might want to use PostgreSQL's notification system for other stuff in the future, so it is not only about caching although it is the first use-case for it. One can easily think of a use case in which we use it to trigger web socket notifications to clients for example. But that's not written in stone.

jarifibrahim · 2018-07-19T06:16:38Z

@kwk
A new migration/update is performed when the spacetemplate is updated. This means that a new pod was spawned and doesn't have the old cache state.
If there are 4 pods they all will be eventually updated. So why do we need to notify the other pods?

I agree there might be an inconsistent state but that wouldn't last for long. As soon as all the pods are updated we will have an empty cache in all the pods.

kwk · 2018-07-19T06:18:32Z

@kwk
A new migration/update is performed when the spacetemplate is updated. This means that a new pod was spawned and doesn't have the old cache state.
If there are 4 pods they all will be eventually updated. So why do we need to notify the other pods?

I agree there might be an inconsistent state but that wouldn't last for long. As soon as all the pods are updated we will have an empty cache in all the pods.

@jarifibrahim in the future the templates can be changed at run-time without requiring a restart.

jarifibrahim · 2018-07-19T06:27:45Z

gormsupport/listener.go

+
+// A SubscriberFunc describes the function signature that a subscriber needs to
+// have. The channel parameter is just an arbitrary identifier string the
+// identities a channel. The extra parameter is can contain optional data that


The extra parameter is can contain

baijum · 2018-09-19T06:43:40Z

main.go

@@ -140,6 +143,13 @@ func main() {
 		os.Exit(0)
 	}

+	// Ensure we delete the work item cache when we receive a notification from postgres
+	gormsupport.SetupDatabaseListener(*config, map[string]gormsupport.SubscriberFunc{


What happens when SetupDatabaseListener function return an error? The error value is not handled here.

Good catch. I will have a look when this PR becomes more relevant.

kwk added 4 commits July 17, 2018 04:53

Invalidate WIT cache on all pods

35ad6db

Add channel to subscriber callback

04d3c8c

Added tests

0bd16a6

Add doc

1bfe26e

kwk added 💣 test database labels Jul 17, 2018

kwk self-assigned this Jul 17, 2018

alien-ike added the work-in-progress label Jul 17, 2018

alien-ike changed the title ~~WIP: Invalidate wit cache~~ Invalidate wit cache Jul 17, 2018

alien-ike removed the work-in-progress label Jul 17, 2018

kwk requested review from aslakknutsen, sbose78 and michaelkleinhenz July 17, 2018 03:30

Test that migration sends the correct signal

6857c09

kwk requested review from baijum, DhritiShikhar, tinakurian and jarifibrahim July 17, 2018 03:39

kwk changed the title ~~Invalidate wit cache~~ WIP: Invalidate wit cache Jul 17, 2018

alien-ike added the work-in-progress label Jul 17, 2018

kwk requested review from xcoulon and alexeykazakov July 17, 2018 03:40

more error checking

5caadbd

kwk commented Jul 17, 2018

View reviewed changes

Fixup

b227b9f

kwk added the space_templates label Jul 17, 2018

baijum requested a review from bartoszmajsak July 17, 2018 07:04

baijum added the 🏠 architecture label Jul 17, 2018

michaelkleinhenz approved these changes Jul 17, 2018

View reviewed changes

Merge branch 'master' into invalidate_wit_cache

68f445a

kwk changed the title ~~WIP: Invalidate wit cache~~ WIP: PostgreSQL LISTEN/NOTIFY: invalidate wit cache Jul 19, 2018

jarifibrahim approved these changes Jul 23, 2018

View reviewed changes

kwk added the idea An idea is a PR that shall not be deleted and captures a brain dump of some sort. label Aug 23, 2018

baijum reviewed Sep 19, 2018

View reviewed changes

tinakurian approved these changes Sep 19, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: PostgreSQL LISTEN/NOTIFY: invalidate wit cache #2172

WIP: PostgreSQL LISTEN/NOTIFY: invalidate wit cache #2172

kwk commented Jul 17, 2018 •

edited

Loading

codecov-io commented Jul 17, 2018 •

edited

Loading

kwk Jul 17, 2018

kwk Jul 17, 2018

kwk Jul 17, 2018

kwk Jul 17, 2018

kwk commented Jul 17, 2018

jarifibrahim commented Jul 17, 2018

baijum commented Jul 17, 2018

kwk commented Jul 17, 2018

jarifibrahim commented Jul 19, 2018

kwk commented Jul 19, 2018

jarifibrahim Jul 19, 2018

baijum Sep 19, 2018

kwk Sep 19, 2018

WIP: PostgreSQL LISTEN/NOTIFY: invalidate wit cache #2172

Are you sure you want to change the base?

WIP: PostgreSQL LISTEN/NOTIFY: invalidate wit cache #2172

Conversation

kwk commented Jul 17, 2018 • edited Loading

codecov-io commented Jul 17, 2018 • edited Loading

Codecov Report

kwk Jul 17, 2018

Choose a reason for hiding this comment

kwk Jul 17, 2018

Choose a reason for hiding this comment

kwk Jul 17, 2018

Choose a reason for hiding this comment

kwk Jul 17, 2018

Choose a reason for hiding this comment

kwk commented Jul 17, 2018

jarifibrahim commented Jul 17, 2018

baijum commented Jul 17, 2018

kwk commented Jul 17, 2018

jarifibrahim commented Jul 19, 2018

kwk commented Jul 19, 2018

jarifibrahim Jul 19, 2018

Choose a reason for hiding this comment

baijum Sep 19, 2018

Choose a reason for hiding this comment

kwk Sep 19, 2018

Choose a reason for hiding this comment

kwk commented Jul 17, 2018 •

edited

Loading

codecov-io commented Jul 17, 2018 •

edited

Loading