Add possibility of creating on the fly the queue needed by a task.#27
Add possibility of creating on the fly the queue needed by a task.#27JeanDaniel-Fischer wants to merge 2 commits into
Conversation
aertje
left a comment
aertje
left a comment
There was a problem hiding this comment.
Hi @JeanDaniel-Fischer, thanks for the PR; I agree it will make a handy addition, but I have some remarks.
It would also be excellent if you can add a simple test that covers the added functionality. I've been pretty slack with tests myself, but now that this project is getting some traction I'd like to see any new stuff covered.
| tsMux sync.Mutex | ||
| } | ||
|
|
||
| var autoCreateQueueOnNewTask = true |
There was a problem hiding this comment.
I'm not in favour of global variables if they can be avoided, mainly as it makes testing harder.
Can we introduce a ServerOptions struct (in Server) instead that holds this flag?
Also I think it should default to false for consistency.
There was a problem hiding this comment.
Yes default should be false, I forget to reset it after a test. I added the ServerOptions struct so we avoid global variable.
|
|
||
| // If auto create queue is on we try to create queue if not existing. | ||
| if !ok && autoCreateQueueOnNewTask { | ||
| createInitialQueue(s, queueName) |
There was a problem hiding this comment.
Using the initial queue method is probably not appropriate here as this will be called in 'normal operation' (not as an initialization). For example, it will display "creating initial queue" (not so relevant), and will panic if it encounters an error.
There was a problem hiding this comment.
I am not sure how to resolve this, I can create a new method but it will duplicate a large portion of the CreateQueue function. I can call CreateQueue function but it would mean create a tasks.CreateQueueRequest inside the CreateTask function and I am not sure it is the best approach.
It seems only the queueName and the parentName are used to create a queue so I could just extract the code of CreateQueue to a function use by both. Maybe you see a better approach ?
There was a problem hiding this comment.
Hi @JeanDaniel-Fischer, apologies for the delay in my response. Yes I see your point;
To understand what we should reuse, can I ask what you expect in terms of error handling? I.e. if the queue name does not meet the required format, do you expect an error of some sort like it would give you if you create the queue separately? Obviously the benefit of reusing CreateQueue is that you can reuse the error checking done there and the error state generated (if any).
|
@JeanDaniel-Fischer this feature could be useful for us too. I borrowed your I could probably take a look at updating & finishing this off, if you'd like? |
|
This would be so useful for us. |
5 participants
Hi,
I am starting to use your emulator but in ours projects we have a lot of queues (more than 10 sometimes). So it is quite
annoying if we have to declare all of them, that is why I propose this merge request that add the -auto-create-queue optional parameter. If set, when a task on an unkown queue arrived, it creates the queue so the task request won't failed.