[wip] User cache support. by SuperSpyTX · Pull Request #13 · riking/marvin

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

+
+	// $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

+		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

+	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

+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

+	"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

+
+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

+	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

+	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

+			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

+	c.ReplaceManyUserObjects(response.Members, true)
+
 	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

 		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

+		err = json.Unmarshal([]byte(data), user)
 		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

+		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


 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

+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

-			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

+	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

+	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

+		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

Conversation

SuperSpyTX commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

riking left a comment

Choose a reason for hiding this comment

Uh oh!

riking Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riking left a comment

Choose a reason for hiding this comment

Uh oh!

riking Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

riking Sep 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperSpyTX commented Sep 15, 2017

Uh oh!

SuperSpyTX commented Sep 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SuperSpyTX commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

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