Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Fix archive conversion by increasing space for span id #894

Merged
merged 2 commits into from
Apr 20, 2018

Conversation

replay
Copy link
Contributor

@replay replay commented Apr 20, 2018

No description provided.

@replay replay requested review from Dieterbe and woodsaj April 20, 2018 17:23
@Dieterbe
Copy link
Contributor

Dieterbe commented Apr 20, 2018

some notes about this bug:

  • any span larger than 3600 overflows the 4 bits of space we have for it. in our case that only affects environments with 2h spans, which overflow to pos 2,the spot of the 5s span (which no one uses)
  • NewAggregator creates sub-AggMetric's, with an AMKey that has overflowed in case of the 2h spans. the in-memory AMKey is effectively one for 5seconds. the only places where the key is used:
  • chunkcache (ephemeral anyway, no prob)
  • cassandra data store (oops -> building migration tool)
  • metricpersist stream (still looking into ramifications)

metricpersist:

  • outgoing: kafka routing not affected (key computed without archive info), but message has wrong key
  • incoming:
    • calls AggMetric.SyncAggregatedChunkSaveState with wrong info, will just ignore it (or update the wrong aggregation entry if you had an overlap, which doesn't apply for us)

when deploying fixed version, it won't be aware of any chunks saved,
as long as we have migrated the data, we can just deploy with drop-first-chunk and we should be fine.

Copy link
Member

@woodsaj woodsaj left a comment

Choose a reason for hiding this comment

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

lgtm

@replay replay merged commit 3039a35 into master Apr 20, 2018
Dieterbe added a commit to raintank/schema that referenced this pull request Jun 7, 2018
@Dieterbe Dieterbe deleted the fix_archive_conversion branch September 18, 2018 08:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants