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

Fallback server checker, parallelized #2617

Merged
merged 7 commits into from
Jun 15, 2015
Merged

Fallback server checker, parallelized #2617

merged 7 commits into from
Jun 15, 2015

Conversation

uaalto
Copy link
Contributor

@uaalto uaalto commented Jun 8, 2015

Based on code by @aranhoide , parallelized, outputs to stdout only on error.

Note: debug logging remains from flashlight/client dependency, it must be removed

@uaalto
Copy link
Contributor Author

uaalto commented Jun 9, 2015

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.

@myleshorton myleshorton closed this Jun 9, 2015
@myleshorton myleshorton reopened this Jun 9, 2015
@myleshorton
Copy link
Contributor

So this is to replace the old fallback server checking script, is that correct @uaalto?

@uaalto
Copy link
Contributor Author

uaalto commented Jun 9, 2015

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.

@aranhoide
Copy link
Contributor

@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.

@aranhoide
Copy link
Contributor

@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.

@aranhoide
Copy link
Contributor

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@uaalto
Copy link
Contributor Author

uaalto commented Jun 10, 2015

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?
In another order of things, if I push to this branch any changes, are the commits "appended" to this pull request or I need to open a new one and reference it somehow?

@aranhoide
Copy link
Contributor

@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.

@uaalto
Copy link
Contributor Author

uaalto commented Jun 10, 2015

Thank you @aranhoide ! will do!

aranhoide added a commit that referenced this pull request Jun 15, 2015
@aranhoide aranhoide merged commit 4ee45ca into valencia Jun 15, 2015
@aranhoide aranhoide deleted the checkfallbacks branch June 15, 2015 21:59
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