TimeSeries samples now Truncated, not Accumulated #5905

mrtracy · 2016-04-06T21:25:49Z

This commit modifies the way our time series system handles multiple values
in the same "sample period". Previously, the engine would accumulate the values
from the individual samples, maintaining a "sum", "count", "max" and "min".

However, there were a variety of consistency issues when this interacted with
the concept of "partial merges":

Our ability to provide protection from replayed raft commands was different,
depending on how partial merges were applied.
The accumulation of floating point values is not associative, and the order
could be different depending on how partial merges are applied.

Most troublingly, this could result on inconsistencies between raft replicas -
partial merges may occur on one replica independently of the others, resulting
in differences in the final merged result on disk. This is unacceptable.

Unfortunately, support for "partial merges" is relatively important for
performance reasons, and we are hesitant to remove it.

To fix all problems associated with this, we have decided to remove sample
accumulation. As an alternative, when multiple samples exist for the same sample
offset, they are discarded except for the most recently merged. This is
considered an acceptable compromise for all time series maintained by cockroach.

Additional Changes:

Updated ts/doc.go to better reflect the current state of the /ts package.
Added a test designed by @bdarnell to test replay protection in the scope of
compactions.
Removed generalized "replay protection" in merge (which did not work) and
added an advisory for any future programmers looking to add a mergeable type.

Fixes #5658

This change is

petermattis · 2016-04-06T21:38:11Z

storage/engine/rocksdb/db.cc

+    // the merge system is for time series data, which is safe against replay;
+    // however, this property is not general for all potential mergeable types.
+    // If a future need arises to merge another type of data, replay protection
+    // will likely need to be a consideration.
    if (left->has_merge_timestamp() && right.has_merge_timestamp()) {
      if (left->merge_timestamp().wall_time() == right.merge_timestamp().wall_time() &&
          left->merge_timestamp().logical() == right.merge_timestamp().logical()) {


Didn't you want to disable the replay protection here?

Ah, never mind. You're saying that merge replays are now safe due to the way merges are processed.

No, you were right the first time, I wanted to remove this. I had a weird merge at the very end, didn't delete enough.

tamird · 2016-04-06T22:29:47Z

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

storage/engine/mvcc_test.go, line 3246 [r1] (raw file):
slightly more type-safe is proto.Equal (reflect.DeepEqual can fail spuriously when one argument is a pointer and the other is not)

storage/engine/mvcc_test.go, line 3248 [r1] (raw file):
this is unmarshalling into a null pointer; no bueno. make {first,second}TS values?

ts/doc.go, line 19 [r1] (raw file):
s/DV/DB/

ts/doc.go, line 26 [r1] (raw file):
s/these/those/

ts/doc.go, line 77 [r1] (raw file):
s/csae/case/

ts/doc.go, line 89 [r1] (raw file):
too many spaces

Comments from Reviewable

vivekmenezes · 2016-04-07T01:24:53Z

can you update internal.proto to say

max and min are no longer populated and are here for backward compatibility
count will always be 1 in the future and is here for backward compatibility.

This will allow us to remove these values in the future

Review status: all files reviewed at latest revision, 8 unresolved discussions.

storage/engine/mvcc_test.go, line 3216 [r1] (raw file):
remove

storage/engine/rocksdb/db.cc, line 661 [r1] (raw file):
yes please delete this code

Comments from Reviewable

mrtracy · 2016-04-07T04:16:12Z

Comment added to internal.proto.

Review status: 5 of 9 files reviewed at latest revision, 8 unresolved discussions.

storage/engine/mvcc_test.go, line 3216 [r1] (raw file):
Done.

storage/engine/mvcc_test.go, line 3246 [r1] (raw file):
Done.

storage/engine/mvcc_test.go, line 3248 [r1] (raw file):
Done.

storage/engine/rocksdb/db.cc, line 661 [r1] (raw file):
Done.

ts/doc.go, line 19 [r1] (raw file):
Done.

ts/doc.go, line 26 [r1] (raw file):
Done.

ts/doc.go, line 77 [r1] (raw file):
Done.

ts/doc.go, line 89 [r1] (raw file):
Done.

Comments from Reviewable

tamird · 2016-04-07T10:40:55Z

Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

roachpb/internal.proto, line 87 [r2] (raw file):
you should actually rename these to indicate their deprecation.

Comments from Reviewable

petermattis · 2016-04-07T12:28:19Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

storage/engine/rocksdb.go, line 307 [r2] (raw file):
Using compaction to test the merge logic feels indirect. On the plus side, it is exercising the actual code paths that will be used in production, but I'm wondering if it would be better to enhance goMerge (which is only used for unit tests) to specify whether a full or partial merge is desired.

Comments from Reviewable

vivekmenezes · 2016-04-07T13:32:25Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

roachpb/internal.proto, line 87 [r2] (raw file):
I think we can leave them as is because they are still being read. Perhaps we should add the DELETED_ prefix to them after say 3 months and remove them from the code.

Comments from Reviewable

@bdarnell

This commit modifies the way our time series system handles multiple values in the same "sample period". Previously, the engine would accumulate the values from the individual samples, maintaining a "sum", "count", "max" and "min". However, there were a variety of consistency issues when this interacted with the concept of "partial merges": + Our ability to provide protection from replayed raft commands was different, depending on how partial merges were applied. + The accumulation of floating point values is not associative, and the order could be different depending on how partial merges are applied. Most troublingly, this could result on inconsistencies between raft replicas - partial merges may occur on one replica independently of the others, resulting in differences in the final merged result on disk. This is unacceptable. Unfortunately, support for "partial merges" is relatively important for performance reasons, and we are hesitant to remove it. To fix all problems associated with this, we have decided to remove sample accumulation. As an alternative, when multiple samples exist for the same sample offset, they are discarded except for the most recently merged. This is considered an acceptable compromise for all time series maintained by cockroach. Additional Changes: + Updated ts/doc.go to better reflect the current state of the /ts package. + Added a test designed by @bdarnell to test replay protection in the scope of compactions. + Removed generalized "replay protection" in merge (which did not work) and added an advisory for any future programmers looking to add a mergeable type. Fixes cockroachdb#5658

bdarnell · 2016-04-07T18:41:36Z

Review status: 9 of 12 files reviewed at latest revision, 3 unresolved discussions.

storage/engine/rocksdb.go, line 307 [r2] (raw file):
Yeah, this does feel fragile. I verified by hand that this caused partial merges to occur but that's kind of an implementation detail and the test isn't confirming that that the compactions are doing anything at all. Adding a test using goMerge with a new flag sounds like a good idea.

Comments from Reviewable

As part of cockroachdb#5905, general replay protection was removed from the engine's merge operator. This is safe in general because the merge operator is only used for Time Series data, which was made replay-safe in that issue. However, TestMerge was still merging string data, and as a result became flaky due to replays. This commit changes TestMerge to use time series data, which is replay-safe. Fixes cockroachdb#5976

…amples It looks like cockroachdb#5905 missed some comments which still maintained the notion that samples with identical offsets were accumulated instead of truncated. This change fixes these comments.

mrtracy assigned vivekmenezes Apr 6, 2016

petermattis reviewed Apr 6, 2016
View reviewed changes

This was referenced Apr 6, 2016

ts: change timeseries floating point data to fixed point #5560

Closed

roachpb: hold all measurements within a timeseries sample #5836

Closed

mrtracy mentioned this pull request Apr 6, 2016

storage: Separate out consistency checks over floating point data #5518

Closed

mrtracy force-pushed the mtracy/time_series_accumulate_discard branch from 7e4c3f4 to 4e2e7fe Compare April 7, 2016 04:16

mrtracy force-pushed the mtracy/time_series_accumulate_discard branch from 4e2e7fe to b1373fb Compare April 7, 2016 17:49

mrtracy merged commit 1c69ea5 into cockroachdb:master Apr 7, 2016

mrtracy deleted the mtracy/time_series_accumulate_discard branch April 7, 2016 18:11

vivekmenezes mentioned this pull request Apr 14, 2016

stability: High-latency cluster failing with errors like "context deadline exceeded" #6046

Closed

mrtracy mentioned this pull request Apr 18, 2016

Fix TestMerge Flakiness #6104

Merged

nvanbenschoten mentioned this pull request Jul 5, 2016

rocksdb: Fix comments about ConsolidateTimeSeriesValue accumulating samples #7622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TimeSeries samples now Truncated, not Accumulated #5905

TimeSeries samples now Truncated, not Accumulated #5905

mrtracy commented Apr 6, 2016

petermattis Apr 6, 2016

petermattis Apr 6, 2016

mrtracy Apr 6, 2016

tamird commented Apr 6, 2016

vivekmenezes commented Apr 7, 2016

mrtracy commented Apr 7, 2016

tamird commented Apr 7, 2016

petermattis commented Apr 7, 2016

vivekmenezes commented Apr 7, 2016

bdarnell commented Apr 7, 2016

TimeSeries samples now Truncated, not Accumulated #5905

TimeSeries samples now Truncated, not Accumulated #5905

Conversation

mrtracy commented Apr 6, 2016

petermattis Apr 6, 2016

Choose a reason for hiding this comment

petermattis Apr 6, 2016

Choose a reason for hiding this comment

mrtracy Apr 6, 2016

Choose a reason for hiding this comment

tamird commented Apr 6, 2016

vivekmenezes commented Apr 7, 2016

mrtracy commented Apr 7, 2016

tamird commented Apr 7, 2016

petermattis commented Apr 7, 2016

vivekmenezes commented Apr 7, 2016

bdarnell commented Apr 7, 2016