-
Notifications
You must be signed in to change notification settings - Fork 89
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
[DO NOT MERGE] feat: switch to memcache-client over memcached #6119
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import zlib from "zlib" | |
import { isArray } from "lodash" | ||
import config from "config" | ||
import { error, verbose } from "./loggers" | ||
import Memcached from "memcached" | ||
import { MemcacheClient } from "memcache-client" | ||
import { createCacheTracer } from "./tracer" | ||
import { createStatsClient } from "./stats" | ||
|
||
|
@@ -18,14 +18,6 @@ const { | |
|
||
const isTest = NODE_ENV === "test" | ||
|
||
const VerboseEvents = [ | ||
"issue", | ||
"failure", | ||
"reconnecting", | ||
"reconnect", | ||
"remove", | ||
] | ||
|
||
const uncompressedKeyPrefix = "::" | ||
const cacheVersion = "v1" | ||
|
||
|
@@ -68,19 +60,17 @@ function createMockClient() { | |
} | ||
|
||
function createMemcachedClient() { | ||
const client = new Memcached(MEMCACHED_URL, { | ||
poolSize: MEMCACHED_MAX_POOL, | ||
}) | ||
VerboseEvents.forEach((event) => { | ||
client.on(event, () => verbose(`[Cache] ${event}`)) | ||
console.log("hey") | ||
const newClient = new MemcacheClient({ | ||
server: { server: MEMCACHED_URL, maxConnections: MEMCACHED_MAX_POOL }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does seem to be an internal maximum on the number of connections made by this client. (I was worried it might be a server option due to its location.) |
||
}) | ||
return client | ||
return newClient | ||
} | ||
|
||
export const client = isTest ? createMockClient() : createMemcachedClient() | ||
|
||
const cacheTracer: ReturnType<typeof createCacheTracer> = isTest | ||
? { get: (x) => x, set: (x) => x, delete: (x) => x } | ||
? { get: (x) => x, set: (x) => x } | ||
: createCacheTracer() | ||
|
||
const statsClient = isTest ? null : createStatsClient() | ||
|
@@ -108,15 +98,18 @@ function _get<T>(key) { | |
reject(err) | ||
} else if (data) { | ||
if (CACHE_COMPRESSION_DISABLED) { | ||
resolve(JSON.parse(data.toString())) | ||
resolve(JSON.parse(data.value.toString())) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. one API change, the returned data is an object with some metadata, |
||
} else { | ||
zlib.inflate(Buffer.from(data, "base64"), (er, inflatedData) => { | ||
if (er) { | ||
reject(er) | ||
} else { | ||
resolve(JSON.parse(inflatedData.toString())) | ||
zlib.inflate( | ||
Buffer.from(data.value, "base64"), | ||
(er, inflatedData) => { | ||
if (er) { | ||
reject(er) | ||
} else { | ||
resolve(JSON.parse(inflatedData.toString())) | ||
} | ||
} | ||
}) | ||
) | ||
} | ||
} else { | ||
reject() | ||
|
@@ -146,7 +139,7 @@ function _set(key, data, options: CacheOptions) { | |
return new Promise<void>((resolve, reject) => { | ||
const payload = JSON.stringify(data) | ||
verbose(`CACHE SET: ${cacheKey(key)}: ${payload}`) | ||
client.set(cacheKey(key), payload, cacheTtl, (err) => { | ||
client.set(cacheKey(key), payload, { lifetime: cacheTtl }, (err) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. slight difference for |
||
err ? reject(err) : resolve() | ||
}) | ||
}).catch(error) | ||
|
@@ -157,7 +150,7 @@ function _set(key, data, options: CacheOptions) { | |
verbose(`CACHE SET: ${cacheKey(key)}: ${payload}`) | ||
|
||
return new Promise<void>((resolve, reject) => { | ||
client.set(cacheKey(key), payload, cacheTtl, (err) => | ||
client.set(cacheKey(key), payload, { lifetime: cacheTtl }, (err) => | ||
err ? reject(err) : resolve() | ||
) | ||
}) | ||
|
@@ -166,13 +159,6 @@ function _set(key, data, options: CacheOptions) { | |
} | ||
} | ||
|
||
const _delete = (key) => | ||
new Promise<void>((resolve, reject) => | ||
client.del(cacheKey(key), (err) => { | ||
err ? reject(err) : resolve() | ||
}) | ||
) | ||
|
||
export default { | ||
get: <T = any>(key: string) => { | ||
return cacheTracer.get(_get<T>(key)) | ||
|
@@ -181,8 +167,4 @@ export default { | |
set: (key: string, data: any, options: CacheOptions = {}) => { | ||
return cacheTracer.set(_set(key, data, options)) | ||
}, | ||
|
||
delete: (key: string) => { | ||
return cacheTracer.delete(_delete(key)) | ||
}, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,5 @@ export function createCacheTracer() { | |
return { | ||
get: createCommand("get"), | ||
set: createCommand("set"), | ||
delete: createCommand("delete"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see any use case in the codebase outside of tests for So perhaps the |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7323,6 +7323,11 @@ lodash.deburr@^4.1.0: | |
resolved "https://registry.yarnpkg.com/lodash.deburr/-/lodash.deburr-4.1.0.tgz#ddb1bbb3ef07458c0177ba07de14422cb033ff9b" | ||
integrity sha1-3bG7s+8HRYwBd7oH3hRCLLAz/5s= | ||
|
||
[email protected]: | ||
version "4.2.0" | ||
resolved "https://registry.yarnpkg.com/lodash.defaults/-/lodash.defaults-4.2.0.tgz#d09178716ffea4dde9e5fb7b37f6f0802274580c" | ||
integrity sha512-qjxPLHd3r5DnsdGacqOMU6pb/avJzdh9tFX2ymgoZE27BmjXrNy/y4LoaiTeAb+O3gL8AfpJGtqfX/ae2leYYQ== | ||
|
||
lodash.find@^4.6.0: | ||
version "4.6.0" | ||
resolved "https://registry.yarnpkg.com/lodash.find/-/lodash.find-4.6.0.tgz#cb0704d47ab71789ffa0de8b97dd926fb88b13b1" | ||
|
@@ -7556,7 +7561,22 @@ [email protected]: | |
resolved "https://registry.yarnpkg.com/media-typer/-/media-typer-0.3.0.tgz#8710d7af0aa626f8fffa1ce00168545263255748" | ||
integrity sha1-hxDXrwqmJvj/+hzgAWhUUmMlV0g= | ||
|
||
[email protected], memcached@^2.2.2: | ||
memcache-client@^1.0.5: | ||
version "1.0.5" | ||
resolved "https://registry.yarnpkg.com/memcache-client/-/memcache-client-1.0.5.tgz#7a2eb1f1924b8a87ef7f80a25f42949da4d1eff7" | ||
integrity sha512-hUADvh+Cju3eNZ5AWGfNhdI/Y4251pc8Hxq+6RryiVNzMeEnXKQ2z+gyBBAPY4SZnC1nOtp2MJUSsnCWB9t6lg== | ||
dependencies: | ||
lodash.defaults "4.2.0" | ||
memcache-parser "^1.0.0" | ||
optional-require "1.1.8" | ||
tslib "2.1.0" | ||
|
||
memcache-parser@^1.0.0: | ||
version "1.0.1" | ||
resolved "https://registry.yarnpkg.com/memcache-parser/-/memcache-parser-1.0.1.tgz#66b8434e1bccd79a6da4bfe5fec41e5aa6f585b8" | ||
integrity sha512-F5TjjHKUNMCWTVfiJD+7M9ecNbBdnK6xifYZ1WL8PoQ/dP9sB37VQV8hPtyuUqJJFj76qY70Ezj9saQ0gw2FJg== | ||
|
||
memcached@^2.2.2: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like the rate limit middleware still brings in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dug through slack for some history and couldn't figure out the state of this, or if it was ever enabled. Ideally, Which reminds me: we must confirm that datadog automatically instruments this alternate library. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do remove |
||
version "2.2.2" | ||
resolved "https://registry.yarnpkg.com/memcached/-/memcached-2.2.2.tgz#68f86ccfd84bcf93cc25ed46d6d7fc0c7521c9d5" | ||
integrity sha1-aPhsz9hLz5PMJe1G1tf8DHUhydU= | ||
|
@@ -8279,6 +8299,13 @@ opentracing@>=0.12.1: | |
resolved "https://registry.yarnpkg.com/opentracing/-/opentracing-0.14.4.tgz#a113408ea740da3a90fde5b3b0011a375c2e4268" | ||
integrity sha512-nNnZDkUNExBwEpb7LZaeMeQgvrlO8l4bgY/LvGNZCR0xG/dGWqHqjKrAmR5GUoYo0FIz38kxasvA1aevxWs2CA== | ||
|
||
[email protected]: | ||
version "1.1.8" | ||
resolved "https://registry.yarnpkg.com/optional-require/-/optional-require-1.1.8.tgz#16364d76261b75d964c482b2406cb824d8ec44b7" | ||
integrity sha512-jq83qaUb0wNg9Krv1c5OQ+58EK+vHde6aBPzLvPPqJm89UQWsvSuFy9X/OSNJnFeSOKo7btE0n8Nl2+nE+z5nA== | ||
dependencies: | ||
require-at "^1.0.6" | ||
|
||
optionator@^0.8.1: | ||
version "0.8.2" | ||
resolved "https://registry.yarnpkg.com/optionator/-/optionator-0.8.2.tgz#364c5e409d3f4d6301d6c0b4c05bba50180aeb64" | ||
|
@@ -9236,6 +9263,11 @@ [email protected], request@^2.83.0: | |
tunnel-agent "^0.6.0" | ||
uuid "^3.3.2" | ||
|
||
require-at@^1.0.6: | ||
version "1.0.6" | ||
resolved "https://registry.yarnpkg.com/require-at/-/require-at-1.0.6.tgz#9eb7e3c5e00727f5a4744070a7f560d4de4f6e6a" | ||
integrity sha512-7i1auJbMUrXEAZCOQ0VNJgmcT2VOKPRl2YGJwgpHpC9CE91Mv4/4UYIUm4chGJaI381ZDq1JUicFii64Hapd8g== | ||
|
||
require-directory@^2.1.1: | ||
version "2.1.1" | ||
resolved "https://registry.yarnpkg.com/require-directory/-/require-directory-2.1.1.tgz#8c64ad5fd30dab1c976e2344ffe7f792a6a6df42" | ||
|
@@ -10409,6 +10441,11 @@ tsconfig-paths@^3.9.0: | |
minimist "^1.2.0" | ||
strip-bom "^3.0.0" | ||
|
||
[email protected]: | ||
version "2.1.0" | ||
resolved "https://registry.yarnpkg.com/tslib/-/tslib-2.1.0.tgz#da60860f1c2ecaa5703ab7d39bc05b6bf988b97a" | ||
integrity sha512-hcVC3wYEziELGGmEEXue7D75zbwIIVUMWAVbHItGPx0ziyXxrOMQx4rQEVEV45Ut/1IotuEvwqPopzIOkDMf0A== | ||
|
||
tslib@^1.10.0: | ||
version "1.13.0" | ||
resolved "https://registry.yarnpkg.com/tslib/-/tslib-1.13.0.tgz#c881e13cc7015894ed914862d276436fa9a47043" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was just making sure this was a singleton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋