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

mmapstorage #1990

Merged
merged 169 commits into from Sep 9, 2018
Merged

mmapstorage #1990

merged 169 commits into from Sep 9, 2018

Conversation

mpranj
Copy link
Member

@mpranj mpranj commented May 13, 2018

Purpose

Introducing the mmapstorage plugin.

A simple storage plugin benchmark is added to compare it to dump. Many storage plugin tests were added to ctest. It does not yet store the OPMPHM data structure, but is compatible when OPMPHM is enabled. Two variants are compiled: mmapstorage, and mmapstorage_crc if zlib is available. The latter enables CRC32 checksum for the critical data (pointers, sizes, ...).

This is not the global cache plugin.

To enable it with cmake one can use -DKDB_DEFAULT_STORAGE=mmapstorage -DKDB_DB_FILE='default.mmap' -DKDB_DB_INIT='elektra.mmap'.

Checklist

  • commit messages are fine ("module: short statement" syntax and refer to issues)
  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see doc/CODING.md)
  • meta data is updated (e.g. README.md of plugins)
  • release notes are updated (doc/news/_preparation_next_release.md)
  • Add flag for CRC checksum in metadata.
  • I added myself to AUTHORS

@mpranj
Copy link
Member Author

mpranj commented Jun 20, 2018

@ingwinlu could you glance over 67ceb69 and see whether that would be the proper way to add a new build job with a different default storage plugin. I basically copied it from the ini job, but just want to make sure that is how you would do it.

@ingwinlu
Copy link
Contributor

@mpranj lgtm

@mpranj
Copy link
Member Author

mpranj commented Jun 20, 2018

Thanks, and thanks for putting all the hard work into the build system!

@mpranj
Copy link
Member Author

mpranj commented Aug 22, 2018

@markus2330 other than the README, no major rewrites of the plugin are planned. The benchmarks will be improved, and a few tests might be added.

I plan to review everything again myself before I mark it as ready. If you want to do a preliminary review of the plugin code, it would make sense for me now.

NB: The checks haven't completed, but if something is broken it is nothing of major concern. Locally everything runs fine.

@markus2330
Copy link
Contributor

It is better if you finish it first! Without a README.md reviewing is difficult.

@mpranj
Copy link
Member Author

mpranj commented Sep 9, 2018

Thank you for the thorough review! I made the requested changes.
Additionally the following was done:

  • added magic data / consistency checks for MmapMetaData (I overlooked this earlier)
  • improved the output format (now CSV with header) of the storage benchmark for easier evaluation
  • added the benchmark results to the news (but not into the plugin README). If you have a better idea how/where to put it, do tell.
  • added myself to the AUTHORS file

If you have further change requests, please tell. Otherwise it can be merged.

@markus2330 markus2330 merged commit b3c9af5 into ElektraInitiative:master Sep 9, 2018
@markus2330
Copy link
Contributor

Thank you! A pleasure to merge such great code.

@mpranj mpranj mentioned this pull request Sep 9, 2018
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants