-
Notifications
You must be signed in to change notification settings - Fork 500
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
Support slow log tailing sidcar for tidb instance #290
Conversation
Signed-off-by: Yeh-lei Wu <rayingecho@gmail.com>
Signed-off-by: Yeh-lei Wu <rayingecho@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.
LGTM
/run-e2e-tests |
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.
LGTM
/run-e2e-tests |
@@ -59,7 +59,11 @@ format = "text" | |||
disable-timestamp = false | |||
|
|||
# Stores slow query log into separated files. | |||
{{- if .Values.tidb.separateSlowLog }} | |||
slow-query-file = "/tmp/log/tidb/slowlog" |
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.
This also needs to be changed to /var/log/tidb/slowlog
.
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.
Good catch.
BTW, this is kind of a code smell but I can't figure out a better solution. I'll document this caveat or any better ideas?
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 mean user may override the configuration by setting tidb.config
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 think we can specify the command line argument -log-slow-query
of tidb-server, Its priority is higher than the configuration file.
charts/tidb-cluster/values.yaml
Outdated
# cpu: 50m | ||
# memory: 10Mi | ||
requests: {} | ||
# cpu: 50m |
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.
We should specify a request. Very few resources should be needed for this.
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.
Is cpu 20m and memory 5MB a good default for both limit and request? (I've tested locally and seems that 10m cpu, 1MB memory is fairly enough)
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.
OK, LGTM.
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 would not use a limit or make the limit a bit higher at least until we have observed it in production..
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.
Agreed, maybe 50Mi memory limit is enough. This is pretty small comparing to TiDB itself.
/run-e2e-tests |
/run-e2e-test |
/run-e2e-tests |
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.
LGTM
* Support slow log tailing sidcar for tidb instance
Arbitrary mount point given by user may be harmful, so we don't allow customizing the slow-query-log file location.
close #231
Signed-off-by: Yeh-lei Wu rayingecho@gmail.com