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

SNS+SQS replaced with Redis #2

Merged
merged 11 commits into from
Jul 2, 2017
Merged

SNS+SQS replaced with Redis #2

merged 11 commits into from
Jul 2, 2017

Conversation

mrtcode
Copy link
Member

@mrtcode mrtcode commented May 18, 2017

This is a preview of new Redis based stream-server version. It seems that it works ok, but still need more testing. And probably need to rewrite some tests.

@mrtcode
Copy link
Member Author

mrtcode commented May 22, 2017

The load test (tests/other/load_test.js), isn't properly finished, but I am also wondering where would be the best place to put it. It's not like a typical test and takes some time to finish also there isn't clear result that can be interpreted as pass/failure so it probably doesn't belong to Mocha tests.

@mrtcode mrtcode closed this May 22, 2017
@mrtcode mrtcode reopened this May 22, 2017
@mrtcode
Copy link
Member Author

mrtcode commented May 25, 2017

The latest commit is an example how we could get API key ID. As with dataserver part, if we decide it's the right way to do this, I will finish the rest.

server.js Outdated
@@ -129,7 +129,7 @@ module.exports = function (onInit) {

if (apiKey) {
let topics = yield zoteroAPI.getAllKeyTopics(apiKey);
apiKey = crypto.createHash('md5').update(apiKey).digest("hex");
apiKey = yield zoteroAPI.getKeyID(apiKey);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this makes sense. We're already making the exact same API request in getAllKeyTopics. Shouldn't we just change that to getKeyInfo and have it return id and topics?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right, if we're sure that getting API key ID in JSON response fits us then I will update everything else. This was an example. Every test must be updated. Also are we going to call it everywhere the same 'apiKey' name, or we want to change it to 'apiKeyID' ?
Because logically they aren't the same.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should call it apiKeyID.

@dstillman
Copy link
Member

For the load test, we could just make it a command-line node script that we run separately.

@mrtcode
Copy link
Member Author

mrtcode commented May 29, 2017

I decided to not do the big renaming and kept apiKeys circulating inside stream-server as it was before. Instead, when apiKeyID message comes from Redis, it is immediately resolved to apiKey. For this I added apiKeyID -> apiKey mapping which is added/removed when subscribing/unsubscribing apiKeys. All 23 tests are passing.

Regarding the load test, it uses some functionality of already existing test utilities (support/ folder). We also need to mock getAllKeyTopics (now it's called getKeyInfo) if we want to tests not only connections, but also how many channels it can handle. Therefore I am wondering where should I put it. Now it's in /test/other/ folder.

SNS+SQS replaced with Redis.
Fixed slow down when instantly dropping many connections.
Fixed memory leak by changing old WebSockets library to uWebSockets.
…js timeout.

'ws' library is required for tests because 'uws' supports only one message listener.
@mrtcode mrtcode force-pushed the redis branch 4 times, most recently from 8f11bb2 to af88515 Compare June 30, 2017 18:07
@dstillman dstillman merged commit fed11b4 into zotero:master Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants