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

POC of Version Store with pluggable backend #586

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

pablojim
Copy link

First implementation of Version Store with pluggable backends. POC in S3 with read, write, soft delete & snapshots using the existing VersionStore chunking and serialisation mechanisms. Append and hard deletes are not implemented.

This implementation stands alone and has no effect on existing functionality. Contains lots of code duplication form the existing functionality. Has limited error checking and cleanup functionality. This PR is mostly for discussion purposes at this point.

@bmoscon
Copy link
Collaborator

bmoscon commented Jul 18, 2018

@pablojim awesome - I'll take a look this week!

@pablojim
Copy link
Author

General implementation notes:

  • Uses forward pointers everywhere. Versions point to segments, Snapshots point to Versions
  • For Version documents native S3 versioning is used. Snapshotting is just asking S3 for the latest version key of all version docs.
  • VersionStore has knowledge of the backingstore while the serialisation classes remain stateless and are handed a backing store for every operation

Random thoughts/possibilities for improvements:

  • Add an abstract VersionStore base class
  • Implement a backward compatible version of the Mongo VersionStore using this abstraction
  • Allow passing of kwargs from all reads and writes to allow customisation for differing backends. e.g. read only certain columns from parquet.
  • Need to integrate the new VersionStore with Arctic and libraries - e.g. tie libraries to store type and some specific configuration
  • Make use of the S3 metadata functionality - especially when writing the segments write metadata about how it was serialised
  • Switch from BSON for the version document serialisation - maybe YAML? Or JSON if we add some date handling.
  • Can we achieve chunk sharing with parquet? So we get fast appends/modifications and lower storage usage. It seems possible but would require deep integration when writing the parquet files.
  • Multithread the S3 uploads & downloads?
  • Handling of different S3 profiles - e.g. multiple S3 endpoints
  • Add error checking and verification of S3 writes?
  • Add cleanup methods and hard deletes as per existing VersionStore
  • Think about fallbacks for parquet serialisation - dataframes in parquet then everything else in pickle?
  • is there any value in hybrid approaches - data on NFS and metadata in S3, Mongo, Oracle. Could use transparent urls for reading segment data e.g. s3:// or file:// Configuration would be complex.

segment_keys = version['segment_keys']
assert len(segment_keys) == 1, "should only be one segment for parquet"
# TODO this is S3 functionality bleeding out of the backing store.
# Currently reading a Pandas dataframe from a parquet bytes array fails as it only takes a file path.
Copy link
Author

Choose a reason for hiding this comment

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

return sorted(dirs, key=mtime)

def delete_symbol(self, library_name, symbol):
"""Soft deletes a symbol - no data is removed, snapshots still work.
Copy link
Contributor

Choose a reason for hiding this comment

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

Snapshots can be done with hard-links which would make deletions safe

@jamesblackburn
Copy link
Contributor

Quite a nice bit of work - any comments on performance of the implementations?

@pablojim
Copy link
Author

@jamesblackburn
From some early results for the parquet store - for reading some large objects there are dramatic improvements - 3 seconds vs 90 seconds. These are probably worst case scenarios for arctic. Write performance is not so dramatically affected. I need to test more though.

There would also be large improvements due to being able to load partial frames e.g. only loading selected columns and row groups. This may help cases such as #609.

Still some work and implementation decisions to do though.

" Does it exist and is versioning enabled?".format(bucket_name))


class S3KeyValueStore(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to have an abstract class KeyValueStore which has the methods for the KV api
and then have e.g. S3KeyValueStore as one concrete implementation maybe in a separate file ?

Copy link
Contributor

Choose a reason for hiding this comment

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

So if e.g. someone wants to implement a KV store on different backing storage, could simply extend the KeyValueStore class and not the S3-specific one ?

version['segment_count'] = len(segment_keys) # on appends this value is incorrect but is updated later on
version['append_size'] = 0
version['append_count'] = 0
version['segment_keys'] = segment_keys
Copy link
Contributor

Choose a reason for hiding this comment

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

aha, this is the forward-pointer implementation, where the version keeps all the segment keys, cool

previous_version = self._backing_store.read_version(self.library_name, symbol)

handler = self._write_handler(version, symbol, data, **kwargs)
handler.write(self._backing_store, self.library_name, version, symbol, data, previous_version, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice to think about decoupling further and have:

  • the VersionStore top level api (looks great, as shown here)
  • a read/write handler being a composite of:
    1. version metadata handling. Metadata to be attached on the version are returned in the form of a dict byt he handler. The handler is not aware of version/version documents, but only handler-specific metadata. The write handler write() expects metadata to be passed, and returns to the to top-level version new metadata, to be attached in the version document.
    2. serialization handler. Can be numpy recarry serializer, Arrow serializer, anything.
    3. segmentation policy. How to segment data, size of segments (upper size bound probably dictated by the backing store)
    4. compression handler
  • As you already have in the code, a backing_store handler, which is responsible for writing individual segments + associated per-segment metadata at the underlying storage.

Having such a well-separated model, one may create custom implementation of individual handler for serialization/segmentation/compression/backing_store.

The logic of e.g. converting a numpy array to a rec array, segmenting, producing byte arrays and finally write chunks, is now very integrated and hard to add different/new implementations.

Copy link
Author

Choose a reason for hiding this comment

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

I thought about this approach. One worry is that it becomes very complicated with an explosion of possible interactions between the different parts which may or may not make sense e.g. mongodb metadata with parquet serialisation with 2Mb chunking on a S3 backend etc. etc.

Each library would then have to be configured with a particular config to work correctly. I think it might be an abstraction too far.

@yschimke
Copy link
Contributor

@pablojim Shame to let this bitrot, can we discuss later this week with @willdealtry, @shashank88

@shashank88
Copy link
Contributor

shashank88 commented Jan 24, 2019

@pablojim Shame to let this bitrot, can we discuss later this week with @willdealtry, @shashank88

Yeah, this seems pretty good, will go through it tonight. Have fixed the merge conflict. Will see if the tests are fine

@yschimke
Copy link
Contributor

If we don’t think this is prod ready or a feature we want to support long term. Maybe we can segregate it is an example and make sure our API allows this sort of flexibility.

Sent with GitHawk

@pablojim
Copy link
Author

If we don’t think this is prod ready or a feature we want to support long term. Maybe we can segregate it is an example and make sure our API allows this sort of flexibility.

Apart from some dependencies it is completely isolated from the rest of Arctic. It's all in the "pluggable" package and duplicates some code from the main APIs.

One option would be to merge it but mark it in the code and documentation as Beta until it is deemed ready for wider use.

@shashank88
Copy link
Contributor

Will move to a contrib directory to unblock this PR, without committing this to be used as a production store.

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.

6 participants