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

Migrate from JSON to Protobuf+Snappy format for index cache #1013

Closed
wants to merge 21 commits into from

Conversation

GiedriusS
Copy link
Member

@GiedriusS GiedriusS commented Apr 5, 2019

Saving the index cache is very, very painful since it does a lot of reflection underneath and it balloons the RAM consumption and it makes much more time. Switch to using to a binary format seamlessly by saving new index caches in a binary Protobuf format with Snappy encoding on top. Migrate them over to the TSDB format during compaction.

Changes

  • Index cache operations were broken out into a separate interface
  • Compactor converts JSON index caches into the Protobuf format + Snappy on top (TBD)
  • Benchmarks for the JSON index cache operations vs. Protobuf/Snappy index cache operations (TBD)

Verification

go verify tests that conversion between JSON and binary works, and that no data is lost.

@bwplotka
Copy link
Member

bwplotka commented Apr 5, 2019

Can we discuss first why flat buffer not proto buffer? (:

@bwplotka
Copy link
Member

bwplotka commented Apr 5, 2019

Essentially why adding another dep

@GiedriusS
Copy link
Member Author

IMO Flatbuffers should be used as they allow random reads and you do not have to read everything into memory and parse before accessing the fields. https://capnproto.org/news/2014-06-17-capnproto-flatbuffers-sbe.html this is a nice comparison from one of the (former?) core developers of protobufs at Google. @bwplotka what's your opinion on this topic? I thought it was already decided

@GiedriusS
Copy link
Member Author

However, protobufs support map types so it would integrate into our codebase in a nicer way. Plus, we read it and write it at once so perhaps indeed it would be nicer to stay with protobufs?

@bwplotka
Copy link
Member

bwplotka commented Apr 6, 2019

I think it was never decided, at least not aware. This task was "move to some flutbuffer/protobuf". Anyway, I don't have strong opinion, but would prefer conistency and reusing existings deps if possible. But happy to discuss. I know protobufs well, but I am newbie in flatbuffers (:

Also I remember @fabxc had this discussion before when designing TSDB index and he decided to NOT use any of those - just raw binary format with varints (: Maybe I can dig out this discussion from somewhere so we can learn from their arguments. (:

@bwplotka
Copy link
Member

bwplotka commented Apr 6, 2019

Also.. if you look on index cache it's actually 1:1 to TSDB index just with series and postings removed. ;>

So maybe it's as easy as stripping out TSDB index? Imagine how was computing this would be. Plus we could reuse exactly the same readers/writers (partially)

@povilasv
Copy link
Member

povilasv commented Apr 9, 2019

Would be great to have some benchmarks prior vs current, as right now we have no idea whether it worked.

@fabxc
Copy link
Collaborator

fabxc commented Apr 13, 2019

Regarding why TSDB uses a custom format:

  • Protobuf has no random access, compresses integers reasonably well (but not sequence-aware)
  • Flatbuf has random access, doesn't really compress
  • Custom format allows us to make tradeoffs between compression and random access as we need it

In hindsight it would've been a good idea to only use a custom format for the hot parts of the index and use flatbuffers for any scaffolding (like the TOC).

@GiedriusS GiedriusS changed the title Migrate from JSON to Flatbuf for index cache Migrate from JSON to the TSDB format for index cache Apr 13, 2019
@GiedriusS
Copy link
Member Author

It seems like it will be impossible to use TSDB format properly for this use-case since Writer.WritePostings checks the series offsets again and sorts them again before writing out. Thus, the only way to write "proper" postings is to add the series data which means extra information in the index cache which we absolutely do not need. If we were to use empty index postings then it would mean that we would lose the Start/End values - they would only different by 4 due to padding as these examples show. So the only way out of this situation is to write out the series data which we do not want to do.

Also, the TSDB code serializes all of the data into pure big-endian numbers. AFAICT no compression is supported in these code paths that we are interested in. From these simple test cases that I have made I can tell that the index cache files do not differ much in size:

$ wc -c index*
     276 index.cache.dat
     397 index.cache.fresh.dat
     415 index.cache.json
    1088 total

Thus, it seems that the best way forward is to re-use protobufs for the index cache files with snappy compression. It would be fine to use protobufs here too since index cache for us is "all-or-nothing" - we do not care about any streaming features that other encoding formats like flatbufs provide. Also, we could reuse the protobuf knowledge/code that we have here as well.

Thoughts, @fabxc @bwplotka? Perhaps I missed something.

@GiedriusS GiedriusS changed the title Migrate from JSON to the TSDB format for index cache Migrate from JSON to Protobuf+Snappy format for index cache May 13, 2019
@bwplotka
Copy link
Member

bwplotka commented Jun 6, 2019

cc my TSDB index superhero @gouthamve friend (:

I need to dive into why reusing index does not work, but I get why you don't have any size improvment. What if we would compress it?

I would think in this case, protobuf + snappy seems like the solution here, but we were just talking with @gouthamve on IRC about some improvements to index itself, wonder if there is something we can improve in index-cache (Table, Symbols, Posting starts).

@bwplotka
Copy link
Member

I looked it through and I think we should actually try the binary format due to mapping (mostly for symbols) and quick load time. Producing such a binary format can be also quicker as we could just do a straight copy of certain elements from the index itself.

Still however loading on-demand those might tricky due to current LabelValue implementation.

@pracucci
Copy link
Contributor

pracucci commented Dec 11, 2019

See also the proposal for moving to index-header binary format (#1839).

@bwplotka
Copy link
Member

bwplotka commented Jan 6, 2020

#1943 replaces this PR as agreed with @GiedriusS

@bwplotka bwplotka closed this Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants