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

feat: deploy response time collector #1521

Merged
merged 19 commits into from
Apr 1, 2022
Merged

feat: deploy response time collector #1521

merged 19 commits into from
Apr 1, 2022

Conversation

aceforeverd
Copy link
Collaborator

@aceforeverd aceforeverd commented Mar 28, 2022 •
edited
Loading

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

close #1234

  • What is the current behavior? (You can also link to an open issue here)

  • What is the new behavior (if this is a feature change)?

@aceforeverd aceforeverd added the enhancement New feature or request label Mar 28, 2022
@aceforeverd aceforeverd added this to the v0.5 milestone Mar 28, 2022
@aceforeverd aceforeverd self-assigned this Mar 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2022 •
edited
Loading

Linux Test Report

       54 files       181 suites   43m 20s ⏱️
  8 187 tests   8 187 ✔️ 0 💤 0 ❌
12 103 runs  12 103 ✔️ 0 💤 0 ❌

Results for commit 126e472.

♻️ This comment has been updated with latest results.

@aceforeverd
Copy link
Collaborator Author

Online Monitoring

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2022 •
edited
Loading

SDK Test Report

  72 files    72 suites   6m 12s ⏱️
172 tests 169 ✔️ 3 💤 0 ❌
212 runs  209 ✔️ 3 💤 0 ❌

Results for commit 126e472.

♻️ This comment has been updated with latest results.

@aceforeverd aceforeverd force-pushed the feat/deploy-response-time-collector branch from 27f7c90 to e1c674d Compare March 29, 2022 02:08
dl239
dl239 reviewed Mar 29, 2022
View reviewed changes
src/tablet/tablet_impl.cc Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022 •
edited
Loading

HybridSE Linux Test Report

       65 files       232 suites   7m 24s ⏱️
18 870 tests 18 870 ✔️ 0 💤 0 ❌

Results for commit 126e472.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 29, 2022 •
edited
Loading

HybridSE Mac Test Report

       65 files       232 suites   7m 14s ⏱️
18 870 tests 18 870 ✔️ 0 💤 0 ❌

Results for commit 126e472.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Mar 29, 2022 •
edited
Loading

Codecov Report

Merging #1521 (126e472) into main (86ae446) will increase coverage by 0.43%.
The diff coverage is 94.93%.

@@             Coverage Diff              @@
##               main    #1521      +/-   ##
============================================
+ Coverage     66.44%   66.88%   +0.43%     
  Complexity      323      323              
============================================
  Files           589      596       +7     
  Lines        111498   112299     +801     
  Branches        994      994              
============================================
+ Hits          74084    75106    +1022     
+ Misses        37207    36986     -221     
  Partials        207      207
Impacted Files Coverage Δ
src/nameserver/name_server_impl.h 100.00% <ø> (ø)
src/nameserver/system_table.h 96.66% <ø> (-0.04%) ⬇️
src/tablet/tablet_impl.h 100.00% <ø> (ø)
src/test/util.h 87.05% <33.33%> (-1.28%) ⬇️
src/nameserver/name_server_impl.cc 38.84% <79.45%> (+0.84%) ⬆️
src/tablet/sp_cache.h 67.21% <80.00%> (+17.21%) ⬆️
...atistics/query_response_time/query_response_time.h 93.54% <93.54%> (ø)
...tistics/query_response_time/query_response_time.cc 94.23% <94.23%> (ø)
src/tablet/tablet_impl.cc 42.10% <96.15%> (+3.08%) ⬆️
hybridse/include/vm/engine_context.h 100.00% <100.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86ae446...126e472. Read the comment docs.

@aceforeverd
feat: add query_response_time statistics collector
bf4ef47
@aceforeverd
feat: add deploy query time collector
ec8d5dc
@aceforeverd
add tests for query response time collector
446c0c1
@aceforeverd
make deploy response time basic runnable
a18b0e3 
but it is error prone
1. not safe between clusters, updating query_response_time might need a
   distributed lock
2. not thread safe neither, since it still allow getting the stats and
   mutate stats parally
3. lack of tests
@aceforeverd
fix some logs
922f8e3
@aceforeverd
refactor query_response_time
6f10b7f
@aceforeverd
test: enable clang thread safety analysis with absl's thread annotation
beaf4cf
@aceforeverd
feat: collect and write deploy response times in nameserver
3f849c8
@aceforeverd
fix comments
933eb9d
@aceforeverd
build: fix finding protobuf on non-standard OS
3401e43 
use cmake based `find_package` to find cmake, which give more control
and returned variables
@aceforeverd
build: don't fail build if can't find rocksdb with cmake targets
b58b7a7
@aceforeverd
fix compile
98a7f04
@aceforeverd
improve NameServerImpl::GetSdkConnection()
a3a4979
@aceforeverd
add test for DEPLOY RESPONSE TIME
ec88d4f
@aceforeverd aceforeverd force-pushed the feat/deploy-response-time-collector branch from bb4803d to ec88d4f Compare March 30, 2022 15:41
@aceforeverd
fix sql_cluster_avilable_test
7e75acd 
add sanity check when trying to delete records from DEPLOY_RESPONSE_TIME
in name_server
zhanghaohit
zhanghaohit approved these changes Apr 1, 2022
View reviewed changes
Copy link
Collaborator

@zhanghaohit zhanghaohit left a comment

Choose a reason for hiding this comment

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

LGTM

@aceforeverd aceforeverd merged commit 06ccd67 into 4paradigm:main Apr 1, 2022
@aceforeverd aceforeverd deleted the feat/deploy-response-time-collector branch April 1, 2022 06:37
@lumianph lumianph mentioned this pull request May 13, 2022
Closed
39 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vagetablechicken vagetablechicken vagetablechicken approved these changes

@zhanghaohit zhanghaohit zhanghaohit approved these changes

@dl239 dl239 dl239 approved these changes

Assignees

@aceforeverd aceforeverd

Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.5
Development

Successfully merging this pull request may close these issues.

feat: deploy query statistics
4 participants
@aceforeverd @dl239 @zhanghaohit @vagetablechicken