Skip to content
This repository was archived by the owner on Oct 28, 2020. It is now read-only.
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions modules/_all/import.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ import (
_ "github.com/riking/marvin/modules/restart"
_ "github.com/riking/marvin/modules/rss"
_ "github.com/riking/marvin/modules/timedpin"
_ "github.com/riking/marvin/modules/usercache"
_ "github.com/riking/marvin/modules/weblogin"
)
115 changes: 115 additions & 0 deletions modules/usercache/database.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package usercache

import (
"encoding/json"

"github.com/pkg/errors"
"github.com/riking/marvin/slack"
"github.com/riking/marvin/slack/rtm"
)

const (
sqlMigrate1 = `CREATE TABLE module_user_cache (
user_id varchar(15) PRIMARY KEY NOT NULL,
data text
)`

sqlGetAllEntries = `SELECT * FROM module_user_cache`

// $1 = slack.UserID
sqlGetEntry = `SELECT data FROM module_user_cache WHERE user_id = $1`

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

@riking riking Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ON CONFLICT DO UPDATE SET data = EXCLUDED.data
😃

Avoids the need for a transaction / checking first.


// $1 = data (json encoded)
// $2 = slack.UserID
sqlUpdateEntry = `UPDATE module_user_cache SET data = $1 WHERE user_id = $2`
)

func (mod *UserCacheModule) GetEntry(userid slack.UserID) (slack.User, error) {
var entry slack.User

var data string
stmt, err := mod.team.DB().Prepare(sqlGetEntry)
if err != nil {
return entry, nil
}
defer stmt.Close()
row := stmt.QueryRow(userid)
err = row.Scan(&data)
if err != nil {
return entry, nil
}
err = json.Unmarshal([]byte(userid), &entry)
if err != nil {
return entry, nil
}
return entry, nil
}

func (mod *UserCacheModule) LoadEntries() error {
stmt, err := mod.team.DB().Query(sqlGetAllEntries)
if err != nil {
return err
}

defer stmt.Close()
for stmt.Next() {
var id string
var data string
var user slack.User
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


err = stmt.Scan(&id, &data)
if err != nil {
return errors.Wrap(err, "error in user cache: obtaining row info")
continue
}
err = json.Unmarshal([]byte(data), &user)
if err != nil {
return errors.Wrap(err, "error in user cache: unmarshal user object")
}
rtmClient := mod.team.GetRTMClient().(*rtm.Client)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

save this outside the for loop

rtmClient.ReplaceUserObject(&user)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method takes a lock - consider batching by hundreds?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
return stmt.Err()
}

func (mod *UserCacheModule) UpdateEntry(userobject slack.User) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_, exists := mod.GetEntry(userobject.ID)

var entrydata []byte
entrydata, err := json.Marshal(&userobject)
if err != nil {
return err
}

var query = sqlAddEntry
if exists != nil {
query = sqlUpdateEntry
}

stmt, err := mod.team.DB().Prepare(query)
if err != nil {
return err
}

defer stmt.Close()
row := stmt.QueryRow(userobject.ID, entrydata)
var id slack.UserID
err = row.Scan(&id)
return err
}

func (mod *UserCacheModule) UpdateEntries(userobjects []*slack.User) error {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see what you mean.

for _, obj := range userobjects {
if obj != nil {
err := mod.UpdateEntry(*obj)
if err != nil {
return err
}
}
}
return nil
}
64 changes: 64 additions & 0 deletions modules/usercache/usercache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package usercache

import (
"sync"

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run goimports

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, remove the newline between the two import blocks.

"fmt"

"github.com/riking/marvin"
"github.com/riking/marvin/slack"
)

type API interface {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

marvin.Module

GetEntry(userid slack.UserID) (slack.User, error)
UpdateEntry(userobject slack.User) error
UpdateEntries(userobjects []*slack.User) error
}

var _ API = &UserCacheModule{}

// ---
func init() {
marvin.RegisterModule(NewUserCacheModule)
}

const Identifier = "usercache"

type UserCacheModule struct {
team marvin.Team

cacheLock sync.Mutex
cacheMap map[slack.UserID]slack.User
}

func NewUserCacheModule(t marvin.Team) marvin.Module {
mod := &UserCacheModule{
team: t,
cacheMap: make(map[slack.UserID]slack.User),
}
return mod
}

func (mod *UserCacheModule) Identifier() marvin.ModuleID {
return Identifier
}

func (mod *UserCacheModule) Load(t marvin.Team) {
t.DB().MustMigrate(Identifier, 1505192548, sqlMigrate1)
t.DB().SyntaxCheck(sqlGetAllEntries, sqlGetEntry, sqlAddEntry, sqlUpdateEntry)
}

func (mod *UserCacheModule) Enable(team marvin.Team) {
go func() {
err := mod.LoadEntries()
if err != nil {
fmt.Errorf("Error whilst updating entries: %s", err.Error())
return
}
}()
}

func (mod *UserCacheModule) Disable(t marvin.Team) {
}
9 changes: 8 additions & 1 deletion slack/controller/team.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"net/url"
"os"
"strconv"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -318,7 +319,13 @@ func (t *Team) SlackAPIPostJSON(method string, form url.Values, result interface
util.LogBadf("Slack API %s error: %s", method, err)
util.LogBadf("Form for %s: %v", method, form)
if slackResponse.SlackError == "ratelimited" {
time.Sleep(1*time.Second)
retryafter := resp.Header.Get("Retry-After")
intp, err := strconv.ParseInt(retryafter, 10, 64)
if err == nil {
time.Sleep(time.Duration(intp) * time.Second)
} else {
time.Sleep(1 * time.Second)
}
}
return errors.Wrapf(err, "Slack API %s", method)
}
Expand Down
24 changes: 24 additions & 0 deletions slack/rtm/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,20 @@ func (c *Client) ReplaceUserObject(obj *slack.User) {
c.MetadataLock.Lock()
defer c.MetadataLock.Unlock()

var cacheApi userCacheAPI
moduleCacheApi := c.team.GetModule("usercache")
if moduleCacheApi != nil {
cacheApi = moduleCacheApi.(userCacheAPI)
}

obj.CacheTS = time.Now()
for i, v := range c.Users {
if v.ID == obj.ID {
c.Users[i] = obj

if cacheApi != nil {
cacheApi.UpdateEntry(*v)
}
return
}
}
Expand All @@ -107,20 +117,34 @@ func (c *Client) ReplaceManyUserObjects(objs []*slack.User) {
c.MetadataLock.Lock()
defer c.MetadataLock.Unlock()

var cacheApi userCacheAPI
moduleCacheApi := c.team.GetModule("usercache")
if moduleCacheApi != nil {
cacheApi = moduleCacheApi.(userCacheAPI)
}

now := time.Now()
for ci, cv := range c.Users {
for ii, iv := range objs {
if iv != nil && cv.ID == iv.ID {
iv.CacheTS = now
c.Users[ci] = iv
objs[ii] = nil

if cacheApi != nil {
cacheApi.UpdateEntry(*iv)
}
}
}
}
for _, iv := range objs {
if iv != nil {
iv.CacheTS = now
c.Users = append(c.Users, iv)

if cacheApi != nil {
cacheApi.UpdateEntry(*iv)
}
}
}
}
Expand Down
13 changes: 11 additions & 2 deletions slack/rtm/membership_info.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"time"

"github.com/pkg/errors"
"github.com/riking/marvin"
"github.com/riking/marvin/slack"
"github.com/riking/marvin/util"
)
Expand All @@ -16,6 +17,14 @@ type membershipRequest struct {
C chan interface{}
}

type userCacheAPI interface {
marvin.Module

GetEntry(userid slack.UserID) (slack.User, error)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

UpdateEntry(userobject slack.User) error
UpdateEntries(userobjects []*slack.User) error
}

func (c *Client) membershipWorker() {
for req := range c.membershipCh {
req.C <- req.F(c.channelMembers)
Expand Down Expand Up @@ -167,13 +176,13 @@ func (c *Client) fillUsersList() {

for response.PageInfo.NextCursor != "" {
c.ReplaceManyUserObjects(response.Members)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

time.Sleep(2*time.Second)
time.Sleep(2 * time.Second)

form.Set("cursor", response.PageInfo.NextCursor)
err := c.team.SlackAPIPostJSON("users.list", form, &response)
if err != nil {
util.LogError(errors.Wrapf(err, "[%s] Could not retrieve users list", c.Team.Domain))
break
continue
}
}
}
Expand Down