celery.node icon indicating copy to clipboard operation
celery.node copied to clipboard

Add simple Redis backend test which reflects the equivalent AMQP test

Open chilts opened this issue 4 years ago • 2 comments

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.)

chilts avatar Mar 01 '21 00:03 chilts

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);
  }```

actumn avatar Mar 02 '21 12:03 actumn

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

chilts avatar Mar 10 '21 20:03 chilts