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.

Sign up for GitHub

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

Jump to bottom

Refactor/component client #859

Merged
merged 13 commits into from
Mar 2, 2021
Merged

Refactor/component client #859

merged 13 commits into from
Mar 2, 2021

Conversation

shhdgit
Copy link
Member

@shhdgit shhdgit commented Feb 23, 2021

Use component client in profiling module.
Should be merged after #855 .

@breezewish
Copy link
Member

breezewish commented Feb 23, 2021 •
edited
Loading

Cool! We have a lot of refactoring work that can be considered if you have further interests :)

Some examples:

  1. We now inline translations for each components. It would be better to manage them in separate files. Here are some examples of how it can be arranged (though this is a WIP branch): https://github.com/breeswish/tidb-dashboard-ui-ng/tree/master/packages/ui/src/CopyLink

  2. We now manage translations using dot separated name tokens. Maybe it would better to separate them by using different i18next namespaces, which can achieve better isolation.

  3. Maybe valuable to discover how use-swr can be used in TiDB Dashboard.

  4. CSS layout refactoring: The way current <Card>, <CardTable>, <CardTabs> works with paddings is pretty tricky. It is even more tricky when it comes to sticky... But I don't have ideas about how it can be better arranged for now :)

  5. esbuild, new webpack, snowpack, etc. Develop performance always matters!

  6. I believe a lot of useReducer is not necessary and the code base can be greatly simplified by using simple states, instead of actions, reducers, state managements, boilerplate codes, etc. Some examples may be Statement, SlowLog and LogSearch. /cc @baurine

Also, feel free to ask me for some recommendations if you want to develop features instead. We have a lot of features to be implemented that will benefit our users a lot!

@shhdgit
Copy link
Member Author

shhdgit commented Feb 24, 2021

Wow, I'm interested in all these listed. I'm going to locate some issue I was found:

  • hmr is strange, not smooth enough
  • sourcemap will freeze the browser, I dont konw why...

May improve some dev experience.

@shhdgit shhdgit force-pushed the refactor/component-client branch from b06f32c to 8b705af Compare February 24, 2021 03:40
@shhdgit shhdgit force-pushed the refactor/component-client branch from 8b705af to 0374499 Compare February 24, 2021 09:49
@shhdgit
Merge branch 'master' into refactor/component-client
6cfe345 
Conflicts:
	pkg/apiserver/profiling/profile.go
@shhdgit shhdgit marked this pull request as ready for review February 24, 2021 15:21
@shhdgit
Copy link
Member Author

shhdgit commented Feb 24, 2021

/cc @breeswish @baurine

@baurine
Copy link
Collaborator

baurine commented Feb 25, 2021

LGTM!

@breezewish
Copy link
Member

@shhdgit Oh, I just merged this PR that causes conflicts to your PR. Could you rename all "pingcap-incubator" to "pingcap"? After that we could merge this PR 🎉

@shhdgit
Copy link
Member Author

shhdgit commented Mar 2, 2021

Yes, I will resolve these conflicts.

shhdgit added 2 commits March 2, 2021 21:59
@shhdgit
Merge branch 'master' into refactor/component-client
1cb17cd 
Conflicts:
	pkg/apiserver/apiserver.go
	pkg/apiserver/profiling/model.go
	pkg/apiserver/profiling/profile.go
	pkg/apiserver/profiling/service.go
@shhdgit
chore: update pkg name
3c69d52
baurine
baurine approved these changes Mar 2, 2021
View reviewed changes
Copy link
Collaborator

@baurine baurine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • baurine

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@breezewish
Copy link
Member

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 2c11d85

@ti-chi-bot ti-chi-bot merged commit b80a838 into pingcap:master Mar 2, 2021
@shhdgit shhdgit mentioned this pull request Mar 11, 2021
Closed
shhdgit added a commit to shhdgit/tidb-dashboard that referenced this pull request May 12, 2021
@shhdgit
Refactor/component client (pingcap#859)
7adaf64 
* feat(ui): add tiflash profiling option

* feat(apiserver): add node type tiflash

* feat(apiserver): tiflash flame graph pingcap#782

* refactor(apiserver): use component client in profiling module

* chore: remove todo

* chore: comment profile option - frequency temporary

* chore: code format

* chore: remove unused params

* chore: update pkg name

* chore: update pkg name
Conflicts:
	pkg/apiserver/apiserver.go
	pkg/tidb/client.go
breezewish pushed a commit that referenced this pull request May 12, 2021
@shhdgit
Release v2021.05.12.1 (#921)
4ee3e39 
* fix(ttlcache): goroutine leak (#892)
* tidb: forwarder only uses tidb whose status is Up (#893)
* keyviz: Add tips for enabled config (#901)
* ui: rocksdb fields (#896)
* monitoring: setup sentry (#895)
* tidb_client: improve behavior when no alive tidb instance (#900)
* feat(stmt): support config maximum number of stmt kept in memory (#914)
* feat: debug api (#898)
* ui: Improve settings description for Statement (#920)
* feat(ui): add tiflash profiling option (#859)
* ui: Add a warning for the debug API (#922)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@breezewish breezewish breezewish left review comments

@baurine baurine baurine approved these changes

Assignees
No one assigned
Labels
status/can-merge status/LGT1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shhdgit @breezewish @baurine @ti-chi-bot