-
Notifications
You must be signed in to change notification settings - Fork 498
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 Dashboard Metrics Storage Key #2483
Conversation
…b-operator into support_dashboard_key
/run-e2e-in-kind |
if err != nil { | ||
return err | ||
} | ||
return tcsm.syncDashboardMetricStorage(tc, tm) |
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.
Status manager should focus on the status update, can we move the etcd key operation to a separate function out of the status manager?
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.
Currently, i think it is not necessary as we only sync etcd key for MonitorRef
. If there are more new actions for ectd operation, it will be ok for me to split it as an independent manager.
pkg/pdapi/pdetcd.go
Outdated
) | ||
|
||
type PDEtcdApi interface { | ||
type PDEtcdClient interface { | ||
// PutKey would put key to the target pd etcd cluster |
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.
why use would
here? At the end of the function, the put/delete operation may not finish?
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 the comment is misleading, I should use will
here.
pkg/pdapi/pdapi.go
Outdated
@@ -72,6 +75,32 @@ func GetTLSConfig(kubeCli kubernetes.Interface, namespace Namespace, tcName stri | |||
return crypto.LoadTlsConfigFromSecret(secret, caCert) | |||
} | |||
|
|||
func (pdc *defaultPDControl) GetPDEtcdClient(namespace Namespace, tcName string, tlsEnabled bool) (PDEtcdClient, error) { | |||
pdc.mutex.Lock() |
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.
Suggest define separate mutex here, it's an independent client with existing PDClient.
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.
updated.
/run-e2e-in-kind |
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
/merge |
Your auto merge job has been accepted, waiting for:
|
/run-all-tests |
@Yisaer merge failed. |
/run-e2e-tests |
1 similar comment
/run-e2e-tests |
/merge |
/run-all-tests |
/run-all-tests |
/run-all-tests |
@Yisaer merge failed. |
/run-cherry-picker |
Signed-off-by: sre-bot <sre-bot@pingcap.com>
cherry pick to release-1.1 in PR #2644 |
What problem does this PR solve?
close #2331
This request sync prometheus and grafana information into pd etcd with tidb nightly verison. I leave alertmanager into ucp. Here is the demo:
Does this PR introduce a user-facing change?: