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

Add simple Redis backend test which reflects the equivalent AMQP test #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chilts
Copy link
Contributor

@chilts chilts commented Mar 1, 2021

Description

Adds a simple Redis backend test which reflects the current equivalent AMQP test.

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Extra test for the Redis backend. No functionality, no docs, no bug fix.

  • What is the current behavior? (You can also link to an open issue here)

Only current behaviour is tested.

  • What is the new behavior (if this is a feature change)?

No additional behaviour has been added.

  • Other information:

A question I had was whether the two Redis results [ "OK", 0 ] from backend.storeResult() should leak out of the backend, or instead should be like the AMQP .storeResult() and just be true? Please let me know your opinion on this and I'd be happy to do a further PR if we want to keep this backend specific enclosed inside the backend. (Note: I haven't yet tried any failures, but I can try that too depending on your thoughts.)

@actumn
Copy link
Owner

actumn commented Mar 2, 2021

Hi.
Thank you for creating this pull request.

About CeleryBackend APIs, the worker should don't care about the promise result value. I suggest that (although not implemented yet) it should reject with some error when resolve fails.

I'm planning to deprecate AMQPBackend since celery does.
So ignoring AMQPBackend#storeResult resolve values, what if it resolves just result as receiving like below?

  public storeResult(
    taskId: string,
    result: any,
    state: string
  ): Promise<any> {
    return this.set(
      `${keyPrefix}${taskId}`,
      JSON.stringify({
        status: state,
        result,
        traceback: null,
        children: [],
        task_id: taskId,
        date_done: new Date().toISOString()
      })
    ).then(() => result);
  }```

@chilts
Copy link
Contributor Author

chilts commented Mar 10, 2021

Apologies for not getting back to this. I'll take a look in the next day or two. Thanks.

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