-
-
Notifications
You must be signed in to change notification settings - Fork 39
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
Added heartbeat method #6
Conversation
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.
Very good, but not perfect as I mentioned.
I approve 👍
def heartbeat(self, bucket, event: Event, pulsetime: float): | ||
endpoint = "buckets/{}/heartbeat?pulsetime={}".format(bucket, pulsetime) | ||
data = event.to_json_dict() | ||
self.dispatch_thread.add_request(endpoint, data) |
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.
The only issue with this is that if aw-server is unavailable it will stack a lot of heatbeats, however we could technically just stack a few events with longer pulsetimes instead once the server is back online.
This is fine though since this issue is also present in the normal non-heartbeat event dispatching, I just wanted to mention that there is room for optimization.
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 is fine.
If one event is 500 bytes and we queue one per second then after one week we 7*24*60*500 = 5040000 bytes = ~5MB of queued events. It think it sounds like a microoptimization that probably adds unnecessary complexity, but one way to do it in a simple way would be to move the heartbeat merging logic (in aw-server) into aw-core and simply call it to get the job done.
I don't think it is important right now, but feel free to do it the way I proposed. :)
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.
It is not the size that matters though, it is the request count. The RequestDispatcher thread is sending requests blocking, so it will take a while. If we assume that each request takes 20ms(back and forth + aw-server processing) and it sends one event per second in 24hrs it will be 86400 requests, it will take 86400*0.0020=172.8s=~2.8min to clear the queue.
It's not an issue since the client will continue running and queueing events while the RequestDispatcher is clearing the queue, but I wouldn't call it an microoptimization. If we later make it possible to have a remote aw-server the response time could be much higher and become a bigger issue, but still not critical.
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.
Ah, fair enough. I'll created an issue.
Fix the issue where the application name are same for some system applications
No description provided.