[wip] User cache support. #13

SuperSpyTX · 2017-09-12T08:28:38Z

Added preliminary support for user caching. When the module is loaded and enabled, it loads the cached database entries and inserts them directly into the RTM object client.users. In addition, any calls to client.ReplaceManyUserObjects and client.ReplaceUserObject will have the cache updated.

In addition to these changes, I added the delay from the Retry-After http header returned if the call was ratelimited.

Questions:

Should I take a different approach? Currently I modified the two functions above to call the user cache module if it's loaded.
Is adding the delay from the Retry-After header a good idea? It does fill the user cache (all 2,600 users) successfully.

Overall, do you think this is a good implementation?

…s disabled.

riking

Looks mostly good! Some perf problems, though.

riking · 2017-09-12T18:40:19Z

modules/usercache/database.go

+
+	// $1 = slack.UserID
+	// $2 = data (json encoded)
+	sqlAddEntry = `INSERT INTO module_user_cache (user_id,data) VALUES ($1, $2)`


ON CONFLICT DO UPDATE SET data = EXCLUDED.data
😃

Avoids the need for a transaction / checking first.

riking · 2017-09-12T18:41:33Z

modules/usercache/database.go

+		if err != nil {
+			return errors.Wrap(err, "error in user cache: unmarshal user object")
+		}
+		rtmClient := mod.team.GetRTMClient().(*rtm.Client)


save this outside the for loop

riking · 2017-09-12T18:41:58Z

modules/usercache/database.go

+	for stmt.Next() {
+		var id string
+		var data string
+		var user slack.User


pointer to slack.User, remove & on line 74. The object's going to live on the heap anyways.

riking · 2017-09-12T18:45:09Z

slack/rtm/membership_info.go

+type userCacheAPI interface {
+	marvin.Module
+
+	GetEntry(userid slack.UserID) (slack.User, error)


GetEntry is never used and can be dropped from the interface.

riking · 2017-09-12T18:45:48Z

modules/usercache/usercache.go

+	"github.com/riking/marvin/slack"
+)
+
+type API interface {


This API type can be dropped, or at least commented with // interface duplicated in rtm package

riking · 2017-09-12T18:46:08Z

modules/usercache/usercache.go

+
+import (
+	"sync"
+


run goimports

Could have sworn goimports was run on this file and this was the result... I'll run it again and see if it produces the same result.

Oh, remove the newline between the two import blocks.

riking · 2017-09-12T18:46:51Z

modules/usercache/database.go

+	return err
+}
+
+func (mod *UserCacheModule) UpdateEntries(userobjects []*slack.User) error {


consider forwarding UpdateEntry to this function instead - you can prepare the statement once and do many inserts.

I was also considering using transactions (.Begin and .Commit).

Oh I see what you mean.

riking · 2017-09-12T18:47:02Z

modules/usercache/database.go

+	return stmt.Err()
+}
+
+func (mod *UserCacheModule) UpdateEntry(userobject slack.User) error {


Should take a pointer - slack.User is a large object.

riking · 2017-09-12T18:48:41Z

modules/usercache/database.go

+			return errors.Wrap(err, "error in user cache: unmarshal user object")
+		}
+		rtmClient := mod.team.GetRTMClient().(*rtm.Client)
+		rtmClient.ReplaceUserObject(&user)


This method takes a lock - consider batching by hundreds?

I was going to do an big array and loading it that way, but I didn't want to consume so much memory. Perhaps maybe I can make this so every 200 entries (like the Slack API) it'll call ReplaceManyUserObjects with an array of 200, then empty and continue.

SuperSpyTX · 2017-09-13T23:40:40Z

slack/rtm/membership_info.go

 	for response.PageInfo.NextCursor != "" {
-		c.ReplaceManyUserObjects(response.Members)


I had to make this change because otherwise the ReplaceManyUserObjects would get called again for the same group of objects retrieved from the last successful query.

And with the new changes, it was also not retrieving all the users.

riking · 2017-09-14T00:32:47Z

modules/usercache/database.go

 	for stmt.Next() {
 		var id string
 		var data string
-		var user slack.User
+		var user *slack.User = &slack.User{}


Leave it at nil, json.Unmarshal(&user) will allocate for you.

riking · 2017-09-14T00:33:42Z

modules/usercache/database.go

 		if err != nil {
-			return errors.Wrap(err, "error in user cache: unmarshal user object")
+			return err


Probably safer to skip erroring rows - we'll re-fetch the data later.

riking · 2017-09-14T00:33:53Z

modules/usercache/database.go

+		arr = append(arr, user)
+		if len(arr) >= 199 {
+			go rtmClient.ReplaceManyUserObjects(arr, false)
+			arr = make([]*slack.User, 200)


The function isn't blocking boot, so just call ReplaceMany directly and use arr = arr[:0] instead.

riking · 2017-09-14T00:35:39Z

modules/usercache/usercache.go

 }

 func (mod *UserCacheModule) Enable(team marvin.Team) {
 	go func() {
+		fmt.Printf("Loading cache entries....\n")


user cache, until we're caching other objects as well

riking · 2017-09-14T00:36:04Z

modules/usercache/database.go

+func (mod *UserCacheModule) UpdateEntry(userobject *slack.User) error {
+	var objarray = make([]*slack.User, 1)
+	objarray[0] = userobject
+	return mod.UpdateEntries(objarray)


mod.UpdateEntries([]*slack.User{userobject})

I was trying to figure out what the easier way was...ugh.

It's type{fields}

riking · 2017-09-14T00:36:45Z

modules/usercache/database.go

-			if err != nil {
-				return err
+			entrydata, err := json.Marshal(obj)
+			if err == nil {


if err != nil { continue } on json marshalling

riking

Make sure to change the callers of rtm.Client.fillUsersList() - it doesn't need to be called on boot because we have the cache.

riking · 2017-09-14T00:40:24Z

slack/rtm/events.go

+	moduleCacheApi := c.team.GetModule("usercache")
+	if moduleCacheApi != nil && updateCache {
+		cacheApi = moduleCacheApi.(userCacheAPI)
+		cacheApi.UpdateEntries(objs)


~~Move this outside the MetadataLock.~~

riking · 2017-09-14T00:40:35Z

slack/rtm/events.go

+	moduleCacheApi := c.team.GetModule("usercache")
+	if moduleCacheApi != nil {
+		cacheApi = moduleCacheApi.(userCacheAPI)
+		cacheApi.UpdateEntry(obj)


~~Same - move outside the lock.~~

SuperSpyTX · 2017-09-15T20:05:28Z

If you would like, I can squash these commits (like the old bukkit days).

SuperSpyTX · 2017-09-15T20:08:27Z

And yes, I've been testing my code.

riking · 2017-09-21T17:59:33Z

modules/usercache/usercache.go

+		delaystr, _, _ := mod.team.ModuleConfig(Identifier).GetIsDefault("delay")
+		timeint, _ := strconv.ParseInt(timestr, 10, 64)
+		var timeres = time.Unix(timeint, 0)
+		delayres, err := time.ParseDuration(delaystr)


this should fall back to default if it fails - const defaultDelay = 72*time.Hour; if err != nil { delayres = defaultDelay }

Fail safe, not dangerous - setting a bad duration would result in reloading the entire thing every hour.

SuperSpyTX · 2018-10-24T16:55:00Z

Is this still an problem a year later?

Joe Krause added 3 commits September 12, 2017 00:49

[wip] User cache support.

944699c

Actually make sure it will continue to fill users if the user cache i…

dcb4b40

…s disabled.

Fix sql migration query formatting

d5ef49a

riking suggested changes Sep 12, 2017

View reviewed changes

riking reviewed Sep 12, 2017

View reviewed changes

Fixed performance issues identified.

b098afa

SuperSpyTX commented Sep 13, 2017

View reviewed changes

riking reviewed Sep 14, 2017

View reviewed changes

Joe Krause added 2 commits September 15, 2017 13:02

Made changes as requested

bb2eecc

Oops.

6d02953

riking reviewed Sep 21, 2017

View reviewed changes

SuperSpyTX mentioned this pull request Jul 6, 2019

Patch user cache SuperSpyTX/marvin-dev#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] User cache support. #13

[wip] User cache support. #13

SuperSpyTX commented Sep 12, 2017 •

edited

Loading

riking left a comment

riking Sep 12, 2017 •

edited

Loading

riking Sep 12, 2017

riking Sep 12, 2017

riking Sep 12, 2017

riking Sep 12, 2017

riking Sep 12, 2017

SuperSpyTX Sep 12, 2017

riking Sep 12, 2017

riking Sep 12, 2017

SuperSpyTX Sep 12, 2017

SuperSpyTX Sep 12, 2017

riking Sep 12, 2017

riking Sep 12, 2017

SuperSpyTX Sep 12, 2017

SuperSpyTX Sep 13, 2017

SuperSpyTX Sep 13, 2017

riking Sep 14, 2017

riking Sep 14, 2017

riking Sep 14, 2017

riking Sep 14, 2017

riking Sep 14, 2017

SuperSpyTX Sep 14, 2017

riking Sep 14, 2017

riking Sep 14, 2017

riking left a comment

riking Sep 14, 2017 •

edited

Loading

riking Sep 14, 2017 •

edited

Loading

SuperSpyTX commented Sep 15, 2017

SuperSpyTX commented Sep 15, 2017

riking Sep 21, 2017

SuperSpyTX commented Oct 24, 2018

		for response.PageInfo.NextCursor != "" {
		c.ReplaceManyUserObjects(response.Members)

[wip] User cache support. #13

Are you sure you want to change the base?

[wip] User cache support. #13

Conversation

SuperSpyTX commented Sep 12, 2017 • edited Loading

riking left a comment

Choose a reason for hiding this comment

riking Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riking left a comment

Choose a reason for hiding this comment

riking Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

riking Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

SuperSpyTX commented Sep 15, 2017

SuperSpyTX commented Sep 15, 2017

Choose a reason for hiding this comment

SuperSpyTX commented Oct 24, 2018

SuperSpyTX commented Sep 12, 2017 •

edited

Loading

riking Sep 12, 2017 •

edited

Loading

riking Sep 14, 2017 •

edited

Loading

riking Sep 14, 2017 •

edited

Loading