-
Notifications
You must be signed in to change notification settings - Fork 131
add window manager within middleware #437
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
base: develop
Are you sure you want to change the base?
Conversation
sent_message_id=sent_message_id, | ||
transport_name=transport_name, | ||
transport_metadata=transport_metadata, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can inherit from vumi.tests.utils.VumiWorkerTestCase
and avoid having to implement mkmsg_*
these days (we should probably clean-up the remaining instances elsewhere in the Vumi tests).
…trolFlag in Transport.outbound
@hodgestar : follow ur idea on MiddlewareControlFlag and just implemented it for Transport outbound. If ok, i'll finish the work and add Dispatcher + Workers PS: sorry i forgot to have a more incremental comit to avoid mixing the refacting and the new features. |
@@ -220,6 +220,7 @@ def _send_failure(f): | |||
d = self._middlewares.apply_consume("outbound", message, | |||
self.transport_name) | |||
d.addCallback(self.handle_outbound_message) | |||
d.addErrback(self._middlewares.control_flag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs to pushed down into _handle
because this level makes it really hard to tell which middleware the MiddlewareControlFlag
was raised from.
@overnin Looking good! Left a couple of comments about pushing the control flow handling down into the handle method (mostly for later use cases like replying). |
@hodgestar : there is now a first handling of control flag in the middleware loop of the stack. Still need to keep a second handling @ transport level in order to maintain the return None failure. I also added a first version of the resume_handling. |
Questions: 2> In BaseMiddleware, should the function resume_handling be remove to have it as independant helper function like setup_middleware_from_config... 3> When resume_handling in a middleware (cf Window Manager Middleware send_outbound), I also need to resume the Worker specific failure, isn't it? Any suggestion on own to implement it nicely over Transport/ApplicationWorker/Dispatcher? |
The window manager component is not adapted to have many instance running at the same time due to the way it choose his Redis key. Basically the monitor consider his windows all key like "windows:xxx" which is great for a unique monitor that will send the message. However in the case of multiple middleware monitoring for different worker u don't want any interference. In order to keep the all redis key naming consistent, seems important to fix naming rules for middleware? like "worker_name:middleware_name:xxxx". |
To fix the key names you should be able to just pass in a suitable redis sub_manager to the WindowManager constructor ( |
@overnin Answers to three questions
|
Is this still being worked on @overnin ? |
Yes sorry i open this pull request some time months ago. I had to put it on I have finish an implementation on my private branch for testing on prod.
I'll try my best to finish next week. Oliv
|
@overnin This branch still alive? |
@overnin is this branch still alive? |
I tried to embed the window manager in a middleware in order to slow down my SMPP transport when the aggregator don't use SMPP's Throttling mechanism. It's a first try and questions are still open and i would need some directions on the 2 following points
1> How to cancel the following middlewareStack and the worker execution? Two options were identified returning None or throwing an exception like "StopPropagation". None is not very explicit which can lead to mistake when deploying pile of middlewares. Also there is this rule already in the code that 'Return value should never be none'. An exception "StopPropagation" is more explicit but i don't get all the implication that it might have.
2> How to resume the middlewareStack execution when a message is due to be send? in my code i directly call the worker to send the message... but then i'm bypassing all the following middlewares of the worker.