Redis backend #38

vamsiikrishna · 2018-07-19T10:35:47Z

No description provided.

dbemiller

Looks good overall. Few small changes needed to merge.

dbemiller · 2018-07-19T12:13:54Z

config/backends.go

+func(cfg *Redis) validateAndLog() {
+	log.Infof("config.backend.redis.host: %s", cfg.Host)
+	log.Infof("config.backend.redis.port: %d", cfg.Port)
+	log.Infof("config.backend.redis.password: %s", cfg.Password)


This is a security risk... passwords shouldn't end up in the app logs.

dbemiller · 2018-07-19T12:16:35Z

Gopkg.toml

@@ -69,6 +69,10 @@
  branch = "master"
  name = "github.com/vrischmann/go-metrics-influxdb"

+[[constraint]]


There should be a Gopkg.lock change associated with this... it should happen if you run dep ensure. Just add & commit the result.

dbemiller · 2018-07-19T12:17:31Z

backends/redis.go

+
+	if err != nil {
+		log.Fatalf("Error creating Redis backend: %v", err)
+		panic("Failed Connecting to Redis.")


log.Fatalf runs os.Exit. You can remove the panic here... it should never happen.

dbemiller · 2018-07-19T12:22:05Z

backends/redis.go

+}
+
+func (redis *Redis) Put(ctx context.Context, key string, value string) error {
+	err := redis.client.Set(key, value, 0).Err()


This isn't a merge blocker, but wouldn't you want a finite & configurable TTL here?

Without a TTL, you'll cache every bid from all over the internet forever.... And since Bidders change their bids based on the time of day, they're generally only usable for 5-10 minutes.

vamsiikrishna · 2018-07-19T18:34:32Z

@dbemiller thanks for reviewing, I have made the changes .
Thanks !

dbemiller

Looks good, thanks ^^.

dbemiller · 2018-07-19T18:35:41Z

config/backends.go

+	log.Infof("config.backend.redis.host: %s", cfg.Host)
+	log.Infof("config.backend.redis.port: %d", cfg.Port)
+	log.Infof("config.backend.redis.db : %d", cfg.Db)
+	log.Infof("config.backend.redis.ttl : %d", cfg.Expiration)


ttl, or expiry? Should be consistent either way.

dbemiller · 2018-07-19T19:45:11Z

Fixes #37

* Redis backend (prebid#38) * Redis backend * Remove logging password * changes * consistent naming when logging + change default backend to memory * Added a config option for aerospike TTLs (prebid#39) * Added a config option for aerospike TTLs. * Removed stray propery. * Add best-effort TTL support to the API (prebid#40) * Updated the docs. * Added TTL and request validation to the PUT contract classes. * Added max_ttl_seconds to the config request limits. * Added Request-based TTLs to the backends. * Moved TTL-limiting into a backend decorator. * Undid some unnecessary changes. * Made aerospike use the configurable default TTL. * Added a default max-ttl config. (prebid#41) * Added aerospike expiry metrics (prebid#42) * Added expiry metrics to aerospike. * Added a measurement to see how many objects are defining their TTL explicitly/ * Fixes issue prebid#43 (prebid#44) * Added Default routing handler added message instead of trowing 404 Error * Added Default routing handler added message instead of trowing 404 Error * Changed Default index message * Allows for providing a key to store caches under (prebid#45) * Allows for providing a key to store caches under * Adds comments for possible future enhancements. * Fixes some logic * Fixes error handling * Allows activating the key setting feature via ENV variables. (prebid#47) * Allows activating the key setting feature via ENV variables. * Sets proper default * Allows fetching custom non-UUID keys when support is enabled (prebid#48) * add license (resolves prebid#49) (prebid#50) * add license * uppercase per convention * using gofrs uuid now (prebid#51) * using gofrs uuid now * toml and lock files corrected * Gopkg.toml library version correction * toml file added a ^3.2.0 andremoved the init variable u1 * changed uuid.NewV4() error code in put.go * Fix logrus pkg path (prebid#53) * Concurrent map usage is not safe (prebid#54) * setting allowOriginFunc option in cors library instance to send requested Origin value in response header (prebid#60) Co-authored-by: Shalmali Patil <shalmali.patil@pubmatic.com> * Fix proper sync.Pool usage (prebid#55) Putting whole objects in a pool only works if you put in pointers. Otherwise the interface{} conversion is still going to cause an allocation. See: https://play.golang.org/p/dDoWFnuf9bb When you put a slice in a pool you also need to put in a pointer to the slice. See: https://play.golang.org/p/yitACT4RLDY * Add TLS support for Redis backend. (prebid#58) * Add TLS support for Redis backend. Basic with skip verify of a certificate. Useful when using self-signed certificates. See https://godoc.org/crypto/tls#Config for a lot more available options. * go fmt on updated files. * Update variables and references to TLS since that seems to be the standard (vs Tls). * Add TLS support for Redis backend. (prebid#58) * Add TLS support for Redis backend. Basic with skip verify of a certificate. Useful when using self-signed certificates. See https://godoc.org/crypto/tls#Config for a lot more available options. * go fmt on updated files. * Update variables and references to TLS since that seems to be the standard (vs Tls). * Fix Prebid Cache Deploy (prebid#63) Co-authored-by: Gus Carreon * Fix pool elements leftover info bug (prebid#64) * Reset put.Puts slice and added benchmarking tests Co-authored-by: Gus Carreon <gcarreongutierrez@vpn-10-75-11-135.nym2.appnexus.com> * Add Prometheus support to Prebid Cache (prebid#57) * Bug fix. Metrics.Type value was not setting any Enabled flag (prebid#65) * Bug fix. Metrics.Type value was not setting any Enabled flag * Scott's feedback corrections Co-authored-by: Gus Carreon <gcarreongutierrez@vpn-10-75-11-135.nym2.appnexus.com> * Add config defaults (prebid#66) Co-authored-by: Gus Carreon <gcarreongutierrez@vpn-10-75-11-135.nym2.appnexus.com> * Add configuration for admin only POST, default route message changing (prebid#71) * Document rate limiter configuration (prebid#73) * Make yaml configuration file to be optional (prebid#67) * OTT-126 - Add TCP stats library and make it switchable (#6) * Update Aerospike Client (prebid#75) * First draft * Renamed to formatAerospikeError() * Travis CI update to Go 1.14 * rolled back constant names * endpoints/get.go refactor, improved GET error logging, and Mansi's feedback * Rolled back viper and logrus updates for the moment * Corrected test descriptions * Removed some comments in backends/aerospike_test.go * Renamed interface and corrected error messages * Renamed aerospike_test.go import to as_types * Mansi's feedback and mayor refactor * Test case fail messages * Corrected variadic params in formatAerospikeError Co-authored-by: Gus Carreon <gcarreongutierrez@vpn-10-75-10-44.nym2.appnexus.com> Co-authored-by: Gus Carreon <gcarreongutierrez@Guss-MacBook-Pro.local> Co-authored-by: Gus Carreon <gcarreongutierrez@vpn-10-75-11-243.nym2.appnexus.com> * Add log entry at trace level for UUIDs (prebid#78) * Improve Get error handling (prebid#76) * add multiple hosts and user/pass support to aerospike backend (prebid#81) Co-authored-by: Joshua Gross <joshua.gross@indexexchange.com> * First draft (prebid#84) * Run validate.sh with github actions (prebid#83) * Swap opened and closed connection counter names (prebid#79) * Improved get metrics to account for Key not found errors (prebid#85) * Adding support for memcache host auto discovery (prebid#82) * Removed azure backend (prebid#88) * OTT-251: Reduce stats key length (#9) * Add metrics to record request-defined TTL (prebid#89) * Remove Get call from Put endpoint (prebid#87) Co-authored-by: Gus Carreon <gcarreongutierrez@Guss-MacBook-Pro.local> * First draft (prebid#92) Co-authored-by: Gus Carreon <gcarreongutierrez@vpn-10-249-98-20.nym2.appnexus.com> * Fix tests Signed-off-by: mohammad-murad <mohammad.murad@pubmatic.com> * Refactor put endpoint and include RecordPutKeyProvided() metric (prebid#90) Co-authored-by: Gus Carreon <gcarreongutierrez@Guss-MacBook-Pro.local> * Add package and release workflows (prebid#95) * Add release and package workflows * Remove v from the tag version expectation in workflows and temporarily remove sontributors list in release draft * Fix syntax error * Fix tag in workflows * Update docker login in workflow * Fix docker secrets * Point to the prebid docker hub namespace * Add back contributors to release drafter * Update docker hub secret key names * Rolledback uppercase in MISSING_KEY type errors (prebid#97) * fix tests Signed-off-by: mohammad-murad <mohammad.murad@pubmatic.com> * Fix failing Actions because of private repo access * Use replace directive for github.com/prebid/prebid-cache * fix github actions Signed-off-by: mohammad-murad <mohammad.murad@pubmatic.com> * Update go.mod and go.sum Signed-off-by: mohammad-murad <mohammad.murad@pubmatic.com>

Redis backend

73bf70f

dbemiller suggested changes Jul 19, 2018

View reviewed changes

dbemiller requested a review from cirla July 19, 2018 12:22

dbemiller assigned cirla Jul 19, 2018

vamsiautomatad added 2 commits July 19, 2018 18:21

Remove logging password

611d5d4

changes

c97d0a8

dbemiller approved these changes Jul 19, 2018

View reviewed changes

cirla approved these changes Jul 19, 2018

View reviewed changes

consistent naming when logging + change default backend to memory

b3bd3ae

dbemiller merged commit aa80a41 into prebid:master Jul 19, 2018

dbemiller mentioned this pull request Jul 19, 2018

Redis backend #37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redis backend #38

Redis backend #38

vamsiikrishna commented Jul 19, 2018

dbemiller left a comment

dbemiller Jul 19, 2018

dbemiller Jul 19, 2018

dbemiller Jul 19, 2018

dbemiller Jul 19, 2018

vamsiikrishna commented Jul 19, 2018

dbemiller left a comment

dbemiller Jul 19, 2018

dbemiller commented Jul 19, 2018

Redis backend #38

Redis backend #38

Conversation

vamsiikrishna commented Jul 19, 2018

dbemiller left a comment

Choose a reason for hiding this comment

dbemiller Jul 19, 2018

Choose a reason for hiding this comment

dbemiller Jul 19, 2018

Choose a reason for hiding this comment

dbemiller Jul 19, 2018

Choose a reason for hiding this comment

dbemiller Jul 19, 2018

Choose a reason for hiding this comment

vamsiikrishna commented Jul 19, 2018

dbemiller left a comment

Choose a reason for hiding this comment

dbemiller Jul 19, 2018

Choose a reason for hiding this comment

dbemiller commented Jul 19, 2018