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

[do not merge] improving + adding features to go client #19

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

elee1766
Copy link

@elee1766 elee1766 commented Apr 26, 2024

hi, im opening this pr just so you can see what im up to.

if you have any questions or wants, please bring them up

i will also ask questions to you if i have them in this PR, if that is okay? @manast

@elee1766
Copy link
Author

elee1766 commented Apr 26, 2024

still a skeleton, but i have placeholders for what i think is the http api?

some of the paths didnt work, and i had to change a ioredis setting as well for my test to pass, i'm not too sure why.

it complained that the maxretrycount for redis connection was null

since the basepath and token is already in the client, i felt it made sense to also use it to dial websocket so i added them, but i havent updated anything to use those functions yet.

also, for some reason, you were passing token with a query parameter, but i don't understand because i couldn't see anywhere in the ts where you were parsing the token as a query param (i only see the authorization header), maybe im missing something

@elee1766
Copy link
Author

elee1766 commented Apr 26, 2024

@manast ok i filled in a bunch of stuff, still not completely done, but let me know if this is on the right path to what you wanted.

i added context.Context to some of the existing wss based functions as well, a few are still missing proper ctx handling tho

@manast manast requested a review from roggervalf April 27, 2024 08:21
@manast
Copy link
Contributor

manast commented Apr 30, 2024

Sorry for the delay in reviewing this, I will do it asap-

Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I saw you spent some time on the WS version of the Proxy. I think it will be more important to focus on the HTTP version first. But still your changes are good to have as we will finalize the WS in the future as well.
Anyway, I wrote some comments. It would be great to have a couple more of tests for this new functionality.

clients/golang/pkg/client/client.go Outdated Show resolved Hide resolved
clients/golang/pkg/client/client.go Outdated Show resolved Hide resolved
clients/golang/pkg/client/client.go Outdated Show resolved Hide resolved
clients/golang/pkg/client/client.go Outdated Show resolved Hide resolved
_, err = c.httpClient.R().
SetBody(jobs).
SetResult(out).
ForceContentType("application/json").
Copy link
Contributor

Choose a reason for hiding this comment

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

resty should be configured to use JSON by default so that it is not required to use ForceContentType here.

Copy link
Author

Choose a reason for hiding this comment

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

i wasn't sure about this because some endpoints seem to return an OK instead of "OK". since the first is not valid JSON i am worried.

clients/golang/pkg/client/client.go Outdated Show resolved Hide resolved
@@ -0,0 +1,26 @@
package proxyapi

type GetJobsResponse struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think maybe it is more standard to have one file per struct instead of having all in this generic file.

Copy link
Author

Choose a reason for hiding this comment

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

ive been splitting these out into files that make sense as ive been getting more familiar with the api.

src/index.ts Outdated
@@ -15,6 +15,7 @@ let workersConnection: Redis | Cluster;
if (config.redis.url) {
connection = new IORedis(config.redis.url, {
retryStrategy: () => 3000,
maxRetriesPerRequest: 20,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is strange because I think this is the default value (20) for maxRetriesPerRequest in IORedis.

Copy link
Author

@elee1766 elee1766 Apr 30, 2024

Choose a reason for hiding this comment

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

yes this was really odd. for some reason it crash on my computer when it is null. maybe it is my bun version?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, I don't think so, what error do you get?

@elee1766
Copy link
Author

elee1766 commented Apr 30, 2024

@manast here is the error when i dont manually specify the ioredis setting. (it in fact tells me to set to not be null)

[1714502403939][INFO] BullMQ Proxy: Worker connected for queue test with concurrency 10
61 |         this.initializing.catch(err => this.emit('error', err));
62 |     }
63 |     checkBlockingOptions(msg, options, throwError = false) {
64 |         if (this.blocking && options && options.maxRetriesPerRequest) {
65 |             if (throwError) {
66 |                 throw new Error(msg);
                           ^
error: BullMQ: Your redis options maxRetriesPerRequest must be null.
      at checkBlockingOptions (/home/a/repo/github.com/taskforcesh/bullmq-proxy/node_modules/bullmq/dist/cjs/classes/redis-connection.js:66:23)
      at new RedisConnection (/home/a/repo/github.com/taskforcesh/bullmq-proxy/node_modules/bullmq/dist/cjs/classes/redis-connection.js:47:13)
      at new QueueBase (/home/a/repo/github.com/taskforcesh/bullmq-proxy/node_modules/bullmq/dist/cjs/classes/queue-base.js:35:27)
      at new Worker (/home/a/repo/github.com/taskforcesh/bullmq-proxy/node_modules/bullmq/dist/cjs/classes/worker.js:34:9)
      at /home/a/repo/github.com/taskforcesh/bullmq-proxy/src/controllers/websocket/worker.ts:27:20
      at openWorker (/home/a/repo/github.com/taskforcesh/bullmq-proxy/src/controllers/websocket/worker.ts:16:34)
      at open (/home/a/repo/github.com/taskforcesh/bullmq-proxy/src/proxy.ts:29:7)
      at /home/a/repo/github.com/taskforcesh/bullmq-proxy/src/fetch-handler.ts:73:9
      at /home/a/repo/github.com/taskforcesh/bullmq-proxy/src/fetch-handler.ts:38:49
error: script "start" exited with code 1

as for websocket - if it's not finalized yet then that's fine. I just saw some issues with the impl and so did some small fixes.

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