@Jjneely merge timeseries #1352

Merged
merged 4 commits into from Oct 12, 2015

Conversation

Projects
None yet
4 participants
@deniszh
Member

deniszh commented Oct 12, 2015

During a rebalance of a consistent hash cluster, after a partition event
on a replication > 1 cluster, or in other cases we might receive
multiple TimeSeries data for a metric key. Merge them together rather
that choosing the "most complete" one.

Contains original work from @jjneely's #1293 and my changes for enable/disable merging through config variable.

jjneely and others added some commits Aug 12, 2015

datalib.py: Merge TimeSeries when more than one returned per key
During a rebalance of a consistent hash cluster, after a partition event
on a replication > 1 cluster, or in other cases we might receive
multiple TimeSeries data for a metric key.  Merge them together rather
that choosing the "most complete" one.
Use a dict to search for already seen series
For large queries this speed things up by an order of magnitude.  I had
a query that pulled 93,000+ series and was taking 34 minutes of the
webapp to search this list.  With a dict instead the same query runs in
45 seconds.
@jjneely

This comment has been minimized.

Show comment
Hide comment
@jjneely

jjneely Oct 12, 2015

This looks great to me. Thanks!

jjneely commented Oct 12, 2015

This looks great to me. Thanks!

@deniszh

This comment has been minimized.

Show comment
Hide comment
@deniszh

deniszh Oct 12, 2015

Member

Tested that on simple 2 node cluster - works now:

  1. Creating local.random.diceroll3 metric on server1 - with values 1..5:
dzhdanov@graph001:/$ for x in 1 2 3 4 5; do echo "local.random.diceroll3 ${x} `date +%s`" | nc -q0 127.0.0.1 2103; sleep 60; done
dzhdanov@graph001:/$ whisper-dump.py /opt/graphite/storage/whisper/local/random/diceroll3.wsp | grep -A 10 'Archive 0 data'
Archive 0 data:
0: 1444641360,          1
1: 1444641420,          2
2: 1444641480,          3
3: 1444641540,          4
4: 1444641600,          5
5: 0,          0
6: 0,          0
7: 0,          0
8: 0,          0
9: 0,          0

After finishing that script run similar on node 2
2. Creating local.random.diceroll3 metric on server2 with values 6..10:

dzhdanov@graph002:~$ for x in 6 7 8 9 10; do echo "local.random.diceroll3 ${x} `date +%s`" | nc -q0 127.0.0.1 2103; sleep 60; done
dzhdanov@graph002:~$ whisper-dump.py /opt/graphite/storage/whisper/local/random/diceroll3.wsp | grep -A 10 'Archive 0 data'
Archive 0 data:
0: 1444641840,          6
1: 1444641900,          7
2: 1444641960,          8
3: 1444642020,          9
4: 1444642080,         10
5: 0,          0
6: 0,          0
7: 0,          0
8: 0,          0
9: 0,          0
  1. Applying patch with REMOTE_STORE_MERGE_RESULTS=True
  2. Checking Graph in Grafana:
    screen shot 2015-10-12 at 12 30 35
  3. Switching to REMOTE_STORE_MERGE_RESULTS=False on both nodes:
    screen shot 2015-10-12 at 12 31 23

Please check and review - @jjneely @bmhatfield @obfuscurity @brutasse @SEJeff ?

Member

deniszh commented Oct 12, 2015

Tested that on simple 2 node cluster - works now:

  1. Creating local.random.diceroll3 metric on server1 - with values 1..5:
dzhdanov@graph001:/$ for x in 1 2 3 4 5; do echo "local.random.diceroll3 ${x} `date +%s`" | nc -q0 127.0.0.1 2103; sleep 60; done
dzhdanov@graph001:/$ whisper-dump.py /opt/graphite/storage/whisper/local/random/diceroll3.wsp | grep -A 10 'Archive 0 data'
Archive 0 data:
0: 1444641360,          1
1: 1444641420,          2
2: 1444641480,          3
3: 1444641540,          4
4: 1444641600,          5
5: 0,          0
6: 0,          0
7: 0,          0
8: 0,          0
9: 0,          0

After finishing that script run similar on node 2
2. Creating local.random.diceroll3 metric on server2 with values 6..10:

dzhdanov@graph002:~$ for x in 6 7 8 9 10; do echo "local.random.diceroll3 ${x} `date +%s`" | nc -q0 127.0.0.1 2103; sleep 60; done
dzhdanov@graph002:~$ whisper-dump.py /opt/graphite/storage/whisper/local/random/diceroll3.wsp | grep -A 10 'Archive 0 data'
Archive 0 data:
0: 1444641840,          6
1: 1444641900,          7
2: 1444641960,          8
3: 1444642020,          9
4: 1444642080,         10
5: 0,          0
6: 0,          0
7: 0,          0
8: 0,          0
9: 0,          0
  1. Applying patch with REMOTE_STORE_MERGE_RESULTS=True
  2. Checking Graph in Grafana:
    screen shot 2015-10-12 at 12 30 35
  3. Switching to REMOTE_STORE_MERGE_RESULTS=False on both nodes:
    screen shot 2015-10-12 at 12 31 23

Please check and review - @jjneely @bmhatfield @obfuscurity @brutasse @SEJeff ?

obfuscurity added a commit that referenced this pull request Oct 12, 2015

@obfuscurity obfuscurity merged commit 6c52d00 into graphite-project:0.9.x Oct 12, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@deniszh deniszh deleted the deniszh:jjneely_merge_timeseries branch Oct 13, 2015

@GoozeyX

This comment has been minimized.

Show comment
Hide comment
@GoozeyX

GoozeyX Jan 26, 2016

Is there any specific reason why this was not merged into the master branch?

GoozeyX commented Jan 26, 2016

Is there any specific reason why this was not merged into the master branch?

@deniszh

This comment has been minimized.

Show comment
Hide comment
@deniszh

deniszh Jan 26, 2016

Member

Yes. The completely different master code in "data retrieval" area. Still need to be ported there - but it will require more patches to port...
Sad but true.

Member

deniszh commented Jan 26, 2016

Yes. The completely different master code in "data retrieval" area. Still need to be ported there - but it will require more patches to port...
Sad but true.

@obfuscurity

This comment has been minimized.

Show comment
Hide comment
@obfuscurity

obfuscurity Jul 25, 2016

Member

@deniszh @jjneely We absolutely need to get this into master. Is there anyone willing to take this on if we find a sponsor?

Member

obfuscurity commented Jul 25, 2016

@deniszh @jjneely We absolutely need to get this into master. Is there anyone willing to take this on if we find a sponsor?

deniszh added a commit to deniszh/graphite-web that referenced this pull request Aug 21, 2016

iksaif added a commit to criteo-forks/graphite-web that referenced this pull request Aug 26, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment