search

actionhero / node-resque

Node.js Background jobs backed by redis.
https://node-resque.actionherojs.com
Apache License 2.0
1.37k stars 151 forks source link

Rename "master" to "leader" #315

Closed evantahler closed 4 years ago

evantahler commented 4 years ago

The scheduler in charge should be the "leader", and the api should reflect this name change.

charlieroth commented 4 years ago

@evantahler / anyone I believe I have this implemented but I am having issues when running npm run test

Similar errors for all the test cases that fail:

scheduler › with specHelper › can provide an error if connection failed

Expected pattern: /ENOTFOUND|ETIMEDOUT/
Received string:  "connect ECONNREFUSED 23.202.231.169:6379"

Final Output:

 FAIL  __tests__/core/scheduler.ts
 FAIL  __tests__/core/worker.ts
 FAIL  __tests__/core/connection.ts
 FAIL  __tests__/core/queue.ts
 PASS  __tests__/plugins/jobLock.ts
 PASS  __tests__/plugins/retry.ts
 PASS  __tests__/plugins/queueLock.ts
 PASS  __tests__/plugins/noop.ts
 PASS  __tests__/plugins/delayedQueueLock.ts
 PASS  __tests__/plugins/custom_plugins.ts
 PASS  __tests__/core/multiWorker.ts (7.19s)

Test Suites: 4 failed, 7 passed, 11 total
Tests:       13 failed, 79 passed, 92 total
Snapshots:   0 total
Time:        10.397s, estimated 13s

Any knowledge that can be shared on how to properly test? Repo: https://github.com/charlieroth/node-resque/tree/master-to-leader

evantahler commented 4 years ago

Thanks! I made your branch into a pull request to make it easier to see your change https://github.com/actionhero/node-resque/pull/323

evantahler commented 4 years ago

So... everything looks good on CI!

The test that is failing is about getting an error when connecting to a server that can't be reached. The only thing going wrong is the error code you are getting is different than the one I'm getting. To fix it, just add ECONNREFUSED the list! I'd bee happy to merge that in as well

glensc commented 4 years ago

from https://github.com/actionhero/node-resque/pull/323#issuecomment-588601129 it was indicated that here's a discussion "why", I don't' see any reasoning behind the change.

evantahler commented 4 years ago

There was not a discussion of why in this thread.

Following the examples of other projects (for example django, python) and the debate and phased solution of redis itself, I chose to switch to a more inclusive naming scheme for this project.

From redis docs:

A note about the word slave used in this man page and command name: Starting with Redis 5 this command: starting with Redis version 5, if not for backward compatibility, the Redis project no longer uses the word slave. Please use the new command REPLICAOF. The command SLAVEOF will continue to work for backward compatibility.