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

Update example-dnscrypt-proxy.toml #1489

Merged
merged 4 commits into from
Oct 21, 2020
Merged

Conversation

lifenjoiner
Copy link
Member

lb_strategy option 'pn' has been implemented, but not documented.

@lifenjoiner
Copy link
Member Author

Ref:

switch lbStrategyStr := strings.ToLower(config.LBStrategy); lbStrategyStr {
case "":
// default
case "p2":
lbStrategy = LBStrategyP2{}
case "ph":
lbStrategy = LBStrategyPH{}
case "fastest":
case "first":
lbStrategy = LBStrategyFirst{}
case "random":
lbStrategy = LBStrategyRandom{}
default:
if strings.HasPrefix(lbStrategyStr, "p") {
n, err := strconv.ParseInt(strings.TrimPrefix(lbStrategyStr, "p"), 10, 32)
if err != nil || n <= 0 {
dlog.Warnf("Invalid load balancing strategy: [%s]", config.LBStrategy)
} else {
lbStrategy = LBStrategyPN{n: int(n)}
}
} else {
dlog.Warnf("Unknown load balancing strategy: [%s]", config.LBStrategy)
}
}

@lifenjoiner lifenjoiner changed the title Update lb_strategy usage Update example-dnscrypt-proxy.toml Oct 10, 2020
@jedisct1
Copy link
Member

Thanks!

p<n> is something that doesn't really make sense. While it was implemented following a request in a GitHub issue, it was not documented because it may eventually be removed.

DEBUG is also intentionally not documented. This is really only useful for developers, and even then, only occasionally to debug specific issues. Debug mode can also change the server's behavior, for example to make execution deterministic, making the application completely insecure for actual usage.

@lifenjoiner
Copy link
Member Author

OK, got it.

But p<n> maybe help in a badly QoS network: user can set it listen several ports with 1 process, then with another local forwarder which can query to all these ports and return the 1st response. As the latency won't be updated before having a result, a greater than count of server ports of n is needed. ph may be too many to get the very slow servers. So if p<n> is removed, it can only be multi-process for this.

We definitely don't set DEBUG in daily use. But when debugging I always forget it. I also saw forgetting this in the issue dialogs while suggesting people to get logs ;p

@jedisct1
Copy link
Member

Well, we can keep pn.

But maybe not DEBUG. People freak out when they see suspicious things in logs, and report irrelevant issues. DEBUG is also a way to avoid this.

@lifenjoiner
Copy link
Member Author

ph may be too many to that will get the very slow servers.

DEBUG:
Yeah, people freak out of suspicious things.
I mean we should document this in somewhere instead of always memorizing it. When we need debug, we can set it right quickly. I don't know a better place than just a comment. I have been puzzled by why don't all <2 work for at least 2 times, then went to read the source code 😂
BTW, on Windows, set a variable in normal console only persists in its session, it won't affect another non-child process. And setting a variable in console could be too complicated to the people freak out of suspicious things. How about other OS?

@jedisct1
Copy link
Member

I mean we should document this in somewhere instead of always memorizing it.

On a Post-It™ 😂

@lifenjoiner
Copy link
Member Author

To be not confusing, which do you prefer, 'p<n>' or 'pn'?

What about:

## If you intend to set it to `< 2` to debug/dev, environment variable `DEBUG` is needed.

Do you think it's too easy for people to trigger it unexpectedly? There are 2 switches required. People should be responsible for what themselves do, rather than blame you. I personally hoping there can be a notice at hand to be friendly to super users and developers. Please~ One more try 😂

BTW:
Please take a look at #1490
Seems you forgot to optimize it while adjusting the ratios.

@jedisct1
Copy link
Member

To be not confusing, which do you prefer, 'p' or 'pn'?

Maybe p<n> or even p immediately followed by a number (ex: p5).

Do you think it's too easy for people to trigger it unexpectedly

Not unexpectedly, but they are still going to complain if odd things are being printed.

@lifenjoiner
Copy link
Member Author

I have done all I can do

I will keep the comment about DEBUG in my own config file.

@jedisct1 jedisct1 merged commit 078f693 into DNSCrypt:master Oct 21, 2020
@jedisct1
Copy link
Member

Thank you!

@DNSCrypt DNSCrypt locked and limited conversation to collaborators Nov 21, 2020
@lifenjoiner lifenjoiner deleted the lb_strategy branch January 2, 2021 11:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants