-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
vttablet: debug/env page to change variables in real-time #7189
Conversation
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
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.
@sougou this looks good to me.
Value string | ||
} | ||
|
||
func debugEnvHandler(tsv *TabletServer, w http.ResponseWriter, r *http.Request) { |
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.
Probably we should reject the request if the method is not POST
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 actually rely on this for my tests, by using GET. Is there a reason to disallow? Also, we seem to be allowing it in other places.
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 saw that, If you change this, you will have to update the tests.
I think the reason is general HTTP conventions. In theory, GET
requests could be cached and shouldn't mutate state.
I think that ideally, we should stop propagating this pattern. Once people start relying on the get will become even more difficult to change it.
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Backport
NO
Status
READY
Description
Feature implemented as specified in RFC.
Also added links to the new page from
/debug/status
Related Issue(s)
Fixes #7171
Fixes #2983
Todos
Deployment Notes
None
Impacted Areas in Vitess
List general components of the application that this PR will affect: