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

Warn on mismatched thriftFramed settings #27

Open
siggy opened this issue Jan 26, 2016 · 7 comments
Open

Warn on mismatched thriftFramed settings #27

siggy opened this issue Jan 26, 2016 · 7 comments

Comments

@siggy
Copy link
Member

siggy commented Jan 26, 2016

two issues:

  • thriftFramed defaults to true at both router-level and server-level.
  • setting thriftFramed to false at the router level does not affect server-level. I'd expect the server to honor whatever is set on the router, unless thriftFramed is explicitly set on the server to override.
@gtcampbell
Copy link
Contributor

I may be able to address this as part of my configuration work - I've been doing some thinking on how we want to handle defaults there.

@gtcampbell
Copy link
Contributor

Are we happy with the current state of this now that server and client are explicitly separated and there is no longer a thriftFramed setting on the router?

@klingerf
Copy link
Member

If it's not a ton of work I think it still might be worthwhile to default thriftFramed and thriftProtocol on the client to to match whatever is set on the server. If somebody sets the server params but not the client params, then the client params should just inherit the server settings.

@adleong
Copy link
Member

adleong commented Mar 11, 2016

I think I disagree. If there are multiple servers, which server should the client inherit from?

Better would be some kind of config linter or warning if your client and server don't match.

@klingerf
Copy link
Member

Fair enough, I forgot about the multiple servers case.

@gtcampbell gtcampbell changed the title fix thriftFramed defaults Warn on mismatched thriftFramed settings Mar 11, 2016
@gtcampbell
Copy link
Contributor

I've changed the title to reflect @adleong's suggestion.

@IssueHuntBot
Copy link

@BoostIO funded this issue with $10. Visit this issue on Issuehunt

Tim-Brooks pushed a commit to Tim-Brooks/linkerd that referenced this issue Dec 20, 2018
Configuration values that take durations are currently specified as
time values with no units.  So `600` may mean 600ms in some contexts and
10 minutes in others.

In order to avoid this problem, this change now requires that
configurations provide explicit units for time values such as '600ms' or
10 minutes'.

Fixes linkerd#27.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

6 participants