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

register events to avoid processing duplicate events #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

boegel
Copy link
Owner

@boegel boegel commented Jul 4, 2024

No description provided.

Copy link

@trz42 trz42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change prevents intentional resending of events, e.g., when developing the bot. Maybe it’s good enough as a temporary workaround.

In any case, I would not hardcode the number of threads to one. I think it would be ok to make it a parameter here and for the bot.

@@ -37,4 +37,5 @@ def handle_issue_comment_event(self, event_info, log_file=None):
if __name__ == '__main__':
app = create_app(klass=ExamplePyGHee)
log("App started!")
waitress.serve(app, listen='*:3000')
# stick to single thread so we can avoid processing duplicate event deliveries multiple times
waitress.serve(app, listen='*:3000', threads=1)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t do that. It could result in the bot becoming unresponsive if the processing of an event takes a long time.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short-term, this is a necessary evil, otherwise the detection of duplicate events can't work, that relies on serially processing events...

By default, threads is 4 with waitress, and to be honest I don't expect big impact of this, since processing a single event shouldn't take much longer than a couple of seconds.

@boegel
Copy link
Owner Author

boegel commented Jul 5, 2024

The change prevents intentional resending of events, e.g., when developing the bot. Maybe it’s good enough as a temporary workaround.

That's true, but I don't see an easy way around that...

For a development scenario just restarting the bot would be enough, since the registered events are only kept in memory.

@Neves-P
Copy link

Neves-P commented Jul 5, 2024

Just chiming in, I've tried running this version of PyGHee on the bot instance at RUG and did a "lame" stress test by sending a bunch of bot commands in quick succession here: Neves-Bot/software-layer#42

I did see waitress warnings in the event_handler but the impact was minimal, I couldn't really tell tasks were waiting for others to finish just by looking at the PR's comments. That said, this was just me messing around in one PR, the traffic on the EESSI repo is higher of course. Nonetheless, even if 3 or 4 people were to submit their commands at once, I wonder if we would notice more than a minor slowdown. Then again, I don't know how much the bot can handle waiting for new tasks.

logging in to /home1/f115372/bot/eessi_bot_event_handler.log
WARNING:waitress.queue:Task queue depth is 1
WARNING:waitress.queue:Task queue depth is 2
WARNING:waitress.queue:Task queue depth is 3
WARNING:waitress.queue:Task queue depth is 4
WARNING:waitress.queue:Task queue depth is 5

@boegel
Copy link
Owner Author

boegel commented Oct 4, 2024

@trz42 How shall we proceed here?

Merge as is, and check if there's trouble due to the changes (on the dev.eessi.io bot for example)?

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.

3 participants