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

[troika-three-text] suppress warnings #156

Closed
peterqliu opened this issue Sep 1, 2021 · 4 comments
Closed

[troika-three-text] suppress warnings #156

peterqliu opened this issue Sep 1, 2021 · 4 comments

Comments

@peterqliu
Copy link

Great module here. Is it possible to suppress WorkerModule warnings, or better yet, have some control over worker calls so users could spread them out a bit?

@lojjic
Copy link
Collaborator

lojjic commented Sep 1, 2021

Probably yes on both accounts...

I originally thought there was value in trying to detect stuck requests and proactively warn the developer about it, to help guide them toward a hard-to-find issue, but I've become less convinced of that over time. It's probably fine to remove it, especially if it's mostly warning when it shouldn't. I'd accept a PR for removing it.

In terms of "spread them out", I'm interpreting that as allowing a pool of workers that can take requests in parallel...? I've experimented a bit with that and I think it's a good option to have available.

@peterqliu
Copy link
Author

In terms of "spread them out", I'm interpreting that as allowing a pool of workers that can take requests in parallel...?

possibly, though I just mean some way to create lots of Text()s in a way that openRequests never gets overwhelmed to spawn warnings in the first place.

I'm not sure why things get stuck (wouldn't it just keep building a backlog?), or why that stuckness is non-deterministic ("some requests may not be returning"). That vagueness makes it unclear if anything actually went wrong.

Happy to do a PR here. As a user, the big win would be avoiding this situation altogether. A medium win would be indicators of hard fails, so that users can implement a fallback. The small win is just to suppress the warnings.

@lojjic
Copy link
Collaborator

lojjic commented Sep 1, 2021

create lots of Text()s

OK, it helps to know that's what is triggering this for you. (I'd try to reduce that number if you can, but that's another topic...)

The intent of the warning is to catch coding errors where some worker code does not properly return results back to the main thread, or gets in an infinite loop tying up the worker thread, or something like that. The "1000 open requests" is just a proxy for detecting that scenario, and it's obviously not an accurate one. Hence the non-deterministic language -- it's guessing that a large number of open requests is because something is stuck and not emptying the queue, but it could also be because there were just a huge number of requests opened all at once and they haven't had a chance to complete yet (as is your case, it sounds like.)

Yes, this could be worked around by introducing another holding queue prior to submitting to the worker, but I think that would really just be adding complexity. The true fault here is in the warning logic; submitting a lot of requests to the worker should be allowed.

So yeah, I think this has convinced me -- either we need to (a) fix the logic to truly detect "stuck" requests and not large numbers of legitimate requests, or (b) quiet the message. I'm totally fine with (b).

@peterqliu
Copy link
Author

currently getting a 403 when trying to push a new branch, but I'll take another look later

@lojjic lojjic closed this as completed in 164fb8f Sep 16, 2021
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

No branches or pull requests

2 participants