Feat/sql index advisor #1460
Merged
Feat/sql index advisor #1460
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #1460 +/- ##
===========================================
+ Coverage 8.93% 24.67% +15.74%
===========================================
Files 121 167 +46
Lines 12682 14972 +2290
===========================================
+ Hits 1133 3695 +2562
+ Misses 11428 11006 -422
- Partials 121 271 +150
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
YiniXu9506
requested review from
shhdgit and
baurine
and removed request for
shhdgit
shhdgit
reviewed
Jan 10, 2023
shhdgit
reviewed
Jan 10, 2023
ui/packages/tidb-dashboard-lib/src/apps/SQLAdvisor/pages/List/index.tsx
Outdated
Show resolved
Hide resolved
ui/packages/tidb-dashboard-lib/src/apps/SQLAdvisor/component/InsightIndexTable.tsx
Outdated
Show resolved
Hide resolved
shhdgit
reviewed
Feb 1, 2023
| @@ -7,7 +7,7 @@ set -euo pipefail | |||
| DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" >/dev/null 2>&1 && pwd )" | |||
| PROJECT_DIR=$(cd "$DIR/../.."; pwd) | |||
|
|
|||
| TARGET="${PROJECT_DIR}/ui/packages/tidb-dashboard-for-op/src/uilts/distro/strings_res.json" | |||
| TARGET="${PROJECT_DIR}/ui/packages/tidb-dashboard-for-op/src/utils/distro/strings_res.json" | |||
| @@ -45,5 +46,9 @@ export default function () { | |||
| return <Monitoring /> | |||
| } | |||
|
|
|||
| if (locHashPrefix === 'sql_advisor') { | |||
There was a problem hiding this comment.
Sync with startsWith after merge master.
| setNoTaskRunning(data) | ||
| return data | ||
| }) | ||
| .catch((e) => console.log(e)) |
There was a problem hiding this comment.
Suggested change
| .catch((e) => console.log(e)) |
Comment on lines
+51
to
+81
| useEffect(() => { | ||
| const checkStatus = async () => { | ||
| const _noTaskRunning = (await taskRunningStatusGet.current()) as boolean | ||
| if (!_noTaskRunning) { | ||
| startCheckTaskStatusLoop.current() | ||
| } | ||
| } | ||
| checkStatus() | ||
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| const checkSQLValidation = async () => { | ||
| await ctx?.ds | ||
| .sqlValidationGet?.() | ||
| .then((res) => { | ||
| setShowAlert(!res) | ||
| }) | ||
| .catch((e) => console.log(e)) | ||
| } | ||
|
|
||
| checkSQLValidation() | ||
| }, [ctx]) |
There was a problem hiding this comment.
How about changing it to the same as taskRunningStatusGet and combining the effect contents?
Suggested change
| useEffect(() => { | |
| const checkStatus = async () => { | |
| const _noTaskRunning = (await taskRunningStatusGet.current()) as boolean | |
| if (!_noTaskRunning) { | |
| startCheckTaskStatusLoop.current() | |
| } | |
| } | |
| checkStatus() | |
| }, []) | |
| useEffect(() => { | |
| const checkSQLValidation = async () => { | |
| await ctx?.ds | |
| .sqlValidationGet?.() | |
| .then((res) => { | |
| setShowAlert(!res) | |
| }) | |
| .catch((e) => console.log(e)) | |
| } | |
| checkSQLValidation() | |
| }, [ctx]) | |
| const checkSQLValidation = useRef(() => { | |
| return ctx?.ds | |
| .sqlValidationGet?.() | |
| .then((res) => { | |
| setShowAlert(!res) | |
| }) | |
| }) | |
| useEffect(() => { | |
| const checkStatus = async () => { | |
| const _noTaskRunning = (await taskRunningStatusGet.current()) as boolean | |
| if (!_noTaskRunning) { | |
| startCheckTaskStatusLoop.current() | |
| } | |
| } | |
| checkStatus() | |
| checkSQLValidation.current() | |
| }, []) |
| }) | ||
| } | ||
| }) | ||
| .catch((e) => console.log(e)) |
There was a problem hiding this comment.
Suggested change
| .catch((e) => console.log(e)) |
| } | ||
| }) | ||
| } catch (e) { | ||
| console.log(e) |
There was a problem hiding this comment.
Do we need notification.error here?
Comment on lines
+178
to
+186
| const registerUserDBStatusGet = async () => { | ||
| await ctx?.ds | ||
| .registerUserDBStatusGet?.() |
There was a problem hiding this comment.
If we use promise.then, we can forget the async/await.
| }, [ctx]) | ||
|
|
||
| const handleDeactivate = async () => { | ||
| await ctx?.ds |
| setIsLoading(false) | ||
| setIsUserDBRegistered(data) | ||
| }) | ||
| .catch((e) => console.log(e)) |
There was a problem hiding this comment.
Do what we indeed have to do to handle errors or ignore them.
Suggested change
| .catch((e) => console.log(e)) |
| }) | ||
| } | ||
| }) | ||
| .catch((e) => console.log(e)) |
There was a problem hiding this comment.
Suggested change
| .catch((e) => console.log(e)) |
baurine
approved these changes
Feb 7, 2023
YiniXu9506
force-pushed
the
feat/sqlIndexAdvisor
branch
from
4ceea28 to
6c07e36
Compare
SabaPing
pushed a commit
to SabaPing/tidb-dashboard
that referenced
this pull request
Apr 4, 2023
* feat: add insight index advisor app * chore: sync upstream master * fix: lint
SabaPing
added a commit
that referenced
this pull request
Apr 4, 2023
* chore: adjust time range limit (#1485) * tweak: change time range by cluster type (#1486) * tweak: change time range by cluster type * fix typo * Feat/sql index advisor (#1460) * feat: add insight index advisor app * chore: sync upstream master * fix: lint * chore: add authorization header for api req (#1490) * chore: add authorization header for api req * chore: bump tidb-dashboard-for-clinic-cloud to 0.0.51 * chore: refine sql advisor (#1488) * chore: refine sql advisor * chore: refine index advisor * fix: lint * refine: aysnc await usage * Revert "upgrade etcd client (#1491)" (#1495) * Hotifx Index Advisor (#1496) * chore: slow query and statements apps refines (#1498) * feat: adjust statement work for serverless clusters (#1500) * feat: adjust statement to work for serverless cluster * chore: bump tidb-dashboard-for-clinic-cloud to 0.0.54 * chore: rename CLINIC_UI_DASHBOARD_PATH to TARGET_VARIANT_DASHBOARD_PATH * fix: fix navigation back miss the selected time range info * chore: support hide the page load progress bar * chore: bump tidb-dashboard-for-clinic-cloud to 0.0.55 * chore: slight refine * chore: bump tidb-dashboard-for-clinic-cloud to 0.0.56 * feat: node metrics (#1501) * add avg by for metrics query (#1503) * update metrics chart (#1502) * update metrics * update version * use fixed pnpm version to build image (#1508) * Fixed issue #1505: Wrong disk mount directory of TiFlash (#1506) * tweak(perf_insight): update apis & workflow (#1504) * update version --------- Co-authored-by: Suhaha <jklopsdfw@gmail.com> Co-authored-by: Yini Xu <34967660+YiniXu9506@users.noreply.github.com> Co-authored-by: Sparkle <1284531+baurine@users.noreply.github.com> Co-authored-by: zzm <zhouzemin@pingcap.com> Co-authored-by: Justin Huang <legendj228@gmail.com> Co-authored-by: Zhenchi <zhongzc_arch@outlook.com> Co-authored-by: Ping An Technology Database Team <109205475+pingandb@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR did?
TODO:
Since the test cluster is in production, the preview env has to hard code the orgId/clusterId/projectId. Update orgId/clusterId/projectId when this PR is ready to merge.