search

grammyjs / runner

Scale bots that use long polling to receive updates.
https://grammy.dev/plugins/runner
MIT License
18 stars 2 forks source link

Modified update fetcher to properly handle some unrecoverable errors #8

Closed slavafomin closed 2 years ago

slavafomin commented 2 years ago

Signed-off-by: Slava Fomin II slava@fomin.io

This should address #7.

slavafomin commented 2 years ago

@KnorpelSenf please check it out and let me know if something needs to be fixed. Maybe some other error codes need to be added that you know of?

I've tested it for 409 error and it seems to work correctly.

KnorpelSenf commented 2 years ago

What do you think about handling all the cases from the core package? I am referring to the code I linked at https://github.com/grammyjs/runner/issues/7#issuecomment-1030649568

slavafomin commented 2 years ago

What do you think about handling all the cases from the core package? I am referring to the code I linked at #7 (comment)

Yep, I'm planning to do this.

By the way, regarding the idea of extracting the error handling logic. Have you considered merging the runner into the core package? I'm not sure why they are separated in the first place.

KnorpelSenf commented 2 years ago

Mostly because we never had a reason to merge them. They're concerned with unrelated tasks. In fact, the runner is a library for arbitrary concurrent long polling of any API. Having this clear SoC helped a lot in the early days when designing the packages.

slavafomin commented 2 years ago

Also, maybe the grammY could use a single polling mechanism out of the box? Is it really worth it to have two separate implementations? I guess it could just confuse new developers with too much choices.

slavafomin commented 2 years ago

What do you think about handling all the cases from the core package?

Do you want to preserve the debug logging for each error code? Or should I just add the error codes to the list?

KnorpelSenf commented 2 years ago

Yes, the having different ways to run the bot account for different groups of library consumers. It's a lesson from Telegraf that there's no silver bullet here, so somehow merging the two strategies is making it worse for both groups.

Also, so far no one has come up with an idea how this could work.

KnorpelSenf commented 2 years ago

What do you think about handling all the cases from the core package?

Do you want to preserve the debug logging for each error code? Or should I just add the error codes to the list?

The debug logs don't need to be as accurate as for the built-in polling.

slavafomin commented 2 years ago

Yes, the having different ways to run the bot account for different groups of library consumers. It's a lesson from Telegraf that there's no silver bullet here, so somehow merging the two strategies is making it worse for both groups.

Also, so far no one has come up with an idea how this could work.

I guess it could be just a single built-in runner with configurable behavior. It will allow to consolidate all the code in one place and reuse the things like error handlers for example.

slavafomin commented 2 years ago

@KnorpelSenf I've just added other error codes from the core package.

KnorpelSenf commented 2 years ago

@slavafomin I fixed this up, can I merge?

KnorpelSenf commented 2 years ago

@slavafomin any updates?