Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Query memcached from dynamo db collector #1602

Merged
merged 1 commit into from
Jun 23, 2016
Merged

Query memcached from dynamo db collector #1602

merged 1 commit into from
Jun 23, 2016

Conversation

jml
Copy link
Contributor

@jml jml commented Jun 20, 2016

  • Extract the report (de)serialization logic to a set of common functions
  • Add options for using memcached to find & store reports
  • Vendor gomemcache client
  • Update timeRequest to parametrize the error -> status behaviour

This change is Reviewable

Namespace: "scope",
Name: "memcache_miss",
Help: "Reports that missed both our in-memory cache and our memcache",
})

This comment was marked as abuse.

This comment was marked as abuse.

@jml
Copy link
Contributor Author

jml commented Jun 20, 2016

Updated. PTAL. I'm going to do some local testing.

@jml
Copy link
Contributor Author

jml commented Jun 20, 2016

Done some more local testing -- everything works. Note: after scaling replicas, SRV resolution seems to fail until I delete & recreate the k8s memcached-svc

@jml jml assigned jml and tomwilkie and unassigned jml Jun 20, 2016
err := updateMemcacheServers(memcachedHost, memcachedService, &memcacheServers)
if err != nil {
// REVIEWER: Undecided whether this should exit or not. Thoughts?
log.Errorf("Could not set memcache servers: %v", err)

This comment was marked as abuse.

This comment was marked as abuse.

@tomwilkie
Copy link
Contributor

@jml I've left quite a few comments, let me know if you want to discuss. Am going to test now, but will assign back to you for the time being to address these comments.

@tomwilkie tomwilkie assigned jml and unassigned tomwilkie Jun 21, 2016
@jml jml force-pushed the memcache-dynamo branch from f524a82 to a94f14c Compare June 22, 2016 11:24
@jml
Copy link
Contributor Author

jml commented Jun 22, 2016

@tomwilkie Thanks for the thorough review. I've responded to all of your comments & have followed your suggestions on most. PTAL.

@jml jml assigned tomwilkie and unassigned jml Jun 22, 2016
@@ -59,6 +54,12 @@ var (
Help: "Size of data read / written from dynamodb.",
}, []string{"method"})

inProcessCacheCounter = prometheus.NewCounterVec(prometheus.CounterOpts{
Namespace: "scope",
Name: "dynamo_cache", // TODO(jml): Rename this monitoring variable.

This comment was marked as abuse.

This comment was marked as abuse.

ticker := time.NewTicker(updateInterval)
go func() {
for range ticker.C {
updateMemcacheServers(host, service, &servers)

This comment was marked as abuse.

This comment was marked as abuse.

@jml jml assigned jml and unassigned tomwilkie and jml Jun 22, 2016
@jml
Copy link
Contributor Author

jml commented Jun 22, 2016

@tomwilkie All responded. PTAL.

@tomwilkie
Copy link
Contributor

LGTM once green.

@tomwilkie
Copy link
Contributor

And a big squash!

If given settings for memcached, services will store & fetch reports
from memcache after checking their in-process cache but before fetching
from S3.
@jml jml force-pushed the memcache-dynamo branch from 7dbf3c9 to 47fcb52 Compare June 22, 2016 17:42
@jml jml merged commit 972847a into master Jun 23, 2016
@jml jml deleted the memcache-dynamo branch June 23, 2016 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants