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.

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

Gears integration (rebased) #653

Merged
merged 39 commits into from
Apr 13, 2021
Merged

Conversation

danni-m
Copy link
Collaborator

@danni-m danni-m commented Feb 17, 2021

Example of running RedisTimeSeries cluster with 15 main nodes and RedisGears

Note: adjust the module's path given your folder struct.

cd /tests/utils
NODES=15 ADDITIONAL_OPTIONS=" --loadmodule ../../../RedisGears/bin/linux-x64-release/redisgears.so --loadmodule  ../../bin/redistimeseries.so" ./create-cluster start
NODES=15 ./create-cluster create
NODES=15 ./create-cluster call RG.REFRESHCLUSTER

@danni-m danni-m force-pushed the gears_integration_reduce_rebased branch 2 times, most recently from 5424284 to 3433358 Compare February 23, 2021 15:23
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #653 (c14fa9b) into master (d95e016) will decrease coverage by 5.02%.
The diff coverage is 73.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #653      +/-   ##
==========================================
- Coverage   92.47%   87.45%   -5.03%     
==========================================
  Files          19       22       +3     
  Lines        2792     3698     +906     
==========================================
+ Hits         2582     3234     +652     
- Misses        210      464     +254     
Impacted Files Coverage Δ
src/generic_chunk.c 81.81% <ø> (ø)
src/resultset.c 98.44% <ø> (+13.35%) ⬆️
src/tsdb.c 95.33% <ø> (+0.51%) ⬆️
src/redisgears.h 41.42% <41.42%> (ø)
src/chunk.c 97.67% <87.50%> (-1.52%) ⬇️
src/gears_integration.c 89.91% <89.91%> (ø)
src/gears_commands.c 90.51% <90.51%> (ø)
src/query_language.c 94.82% <94.50%> (+0.78%) ⬆️
src/module.c 88.82% <97.01%> (+0.12%) ⬆️
src/indexer.c 97.74% <98.61%> (+1.57%) ⬆️
... and 7 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 d95e016...c14fa9b. Read the comment docs.

@filipecosta90 filipecosta90 self-requested a review March 8, 2021 23:53
@filipecosta90
Copy link
Contributor

filipecosta90 commented Mar 9, 2021

A quick summary on the single shard master vs single shard based on this branch read performance ( using tsbs scale 100 ).
Bellow you can find the overall average latency ( including RTT ) per query type.

queryN Query type master (3e45077) gears_integration_reduce_rebased ( f14409b ) master vs branch
1 single-groupby-1-1-1 3.830 3.130 18%
2 single-groupby-1-1-12 16.080 15.370 4%
3 single-groupby-1-8-1 38.180 36.840 4%
4 single-groupby-5-1-1 39.600 38.720 2%
5 single-groupby-5-1-12 200.690 199.960 0%
6 single-groupby-5-8-1 203.390 201.510 1%
7 cpu-max-all-1 37.130 35.320 5%
8 cpu-max-all-8 310.260 298.000 4%
9 double-groupby-1 467.410 451.760 3%
10 double-groupby-5 2435.140 2427.540 0%
11 double-groupby-all 4757.670 4671.970 2%
14 lastpoint 1221.670 1174.480 4%
15 groupby-orderby-limit 331.810 317.100 4%

@danni-m, as measured above the gears_integration_reduce_rebased changes did not affect performance even when not using gears and a single shard.

@lgtm-com
Copy link

lgtm-com bot commented Mar 16, 2021

This pull request introduces 1 alert when merging ede8fc9 into a22d718 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

@danni-m danni-m force-pushed the gears_integration_reduce_rebased branch from 23191c8 to 68d2a00 Compare March 17, 2021 07:46
@lgtm-com
Copy link

lgtm-com bot commented Mar 17, 2021

This pull request introduces 1 alert when merging 68d2a00 into 57f42f0 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 1 alert when merging cd7e495 into ac9a5f5 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 1 alert when merging 373fca0 into ac9a5f5 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 1 alert when merging 12ef2cf into ac9a5f5 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 1 alert when merging 0236770 into d30fda3 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

@filipecosta90 filipecosta90 marked this pull request as ready for review April 1, 2021 20:01
@filipecosta90 filipecosta90 linked an issue Apr 2, 2021 that may be closed by this pull request
10 tasks
filipecosta90
filipecosta90 previously approved these changes Apr 12, 2021
@filipecosta90 filipecosta90 self-requested a review April 13, 2021 14:06
@danni-m danni-m merged commit c495397 into master Apr 13, 2021
@danni-m danni-m deleted the gears_integration_reduce_rebased branch April 13, 2021 15:04
@filipecosta90 filipecosta90 added this to In progress in 1.6 via automation Jul 4, 2021
@filipecosta90 filipecosta90 moved this from In progress to Done in 1.6 Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
1.6
  
Done
Development

Successfully merging this pull request may close these issues.

Multi-Series GROUPBY and REDUCE support
2 participants