-
Notifications
You must be signed in to change notification settings - Fork 11.2k
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
Fallback server checker, parallelized #2617
Conversation
One thing that might require attention: is fmt thread-safe? When testing this program I didn't encounter any instance of where the output was interwoven, but I'm finding it now in other code (rarely, but still). If this is the case, I should change fmt for log. |
So this is to replace the old fallback server checking script, is that correct @uaalto? |
This is indeed a fallback server checking script, optimized to perform the checks concurrently. I'm not sure if the intention is to replace the old script entirely. It is intended for automated checks in a server, that's why it tries to minimize output unless a verbosity flag is set. Maybe @aranhoide can elaborate on the specifics of its deployment, however. |
@myleshorton : Yes, this is intended to replace the old checker, which is based on lantern-java and was configured through a Google Apps app. Also, that one took too long to run a pass of tests (basically because we were testing the proxies serially) and gave false positives too often, for whatever reason. This fits more seamlessly with our current toolchain, since it takes the same format as genconfig does for the fallbacks list (fallbacks.json) and it uses the same code the Lantern client uses to proxy through fallbacks. Also, we expect it to be faster, less memory-hungry, and more reliable. |
@uaalto : No, fmt is not thread-safe. log writes to stderr, while I'd rather have this script output to stdout (this being a nice way to tell library-output logs from program output too). So maybe errors should be collected somehow to be printed in a single (the main?) goroutine. I'll first review the rest of your code and then maybe suggest other ideas about this. |
And btw, this is really based on code by @oxtoacart ; I merely extracted the fallback checking code from genconfig.go and changed it to check the response, in addition to checking that the request completed without error. |
|
||
numcores := runtime.NumCPU() | ||
|
||
if *verbose { |
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.
Nice, although if log debug goes to stderr maybe we don't need to care about limiting log output.
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 checked what log.Debug does, and it sends it to stdout. log.Error sends it to stderr.
In this scenario, I think it would be best to log messages that need to be processed on stdout, and the rest on stderr.
Thank you @aranhoide! I think sending errors through a channel is a nice way of serializing them. It's the first idea I had when it first occured to me that fmt might be thread-unsafe. It isn't that obvious, because output of this script is clean and appears serialized. Log module is thread-safe, though. Does Lantern have a thread-safe logging facility? |
@uaalto : the log module is indeed thread-safe, but log output goes to stderr. We could have the caller of this script read stderr, but I think it's more proper to just write the fallback errors (program output) to stdout, and then we don't have to worry about interference from logs in this program or any libraries it uses. Pushing changes to this branch will update the PR. I'm done with the first pass of code review. When you're ready for another one, just push your changes and ping me. |
Thank you @aranhoide ! will do! |
…ncurrent goroutines
flashlight doesn't really depend on it in any way, and this was polluting the `main` namespace of the genconfig package, causing tests to fail.
Based on code by @aranhoide , parallelized, outputs to stdout only on error.
Note: debug logging remains from flashlight/client dependency, it must be removed