Skip to content

Fix compression format: needs to have the zlib header around deflate. #63

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

Merged
merged 3 commits into from
Oct 10, 2017

Conversation

marshallpierce
Copy link
Collaborator

Also add an example cli tool to make it easy to construct serialized
histograms of various flavors. Seems like it might come in handy for
someone beyond me.

Fixes #62.

Also add an example cli tool to make it easy to construct serialized
histograms of various flavors. Seems like it might come in handy for
someone beyond me.
@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

Hmm, tests/serialization.rs fails to compile: https://travis-ci.org/jonhoo/hdrsample/jobs/285870400#L810

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

Also, for the command-line tool, it'd be nice to also have an argument to turn on auto-resize.
Apart from that LGTM.

@algermissen
Copy link

algermissen commented Oct 10, 2017

Thanks for doing this so quickly!

I just tried the branch version:

  • Added impl Counter for i32 locally

  • produced binary with

    let mut buf = File::create("foo2.hist").expect("ok io");
    let nbytes = V2DeflateSerializer::new()
      .serialize(&hist, &mut buf).expect("ok");
    buf.sync_all().expect("OK");
    
  • The binary is attached: foo2.hist.zip

  • The base64 version is HISTFAAAAFl4AS2KOw5AUBREr+OJiOIVopOIiFKh0ShQ2JfCDizD3sQS3PeZTOYkM9NcdyUimwQRmfic33P9QvHgZBCMmhQLGYUiV1pKWhZf73Qw6ESt74ORnokfyTUHhg==

  • Created hgrm text file and imported that into http://hdrhistogram.github.io/HdrHistogram/plotFiles.html

Result:

Thanks a lot!

@marshallpierce
Copy link
Collaborator Author

Ah, there's a build with no default features. I'll add cfg as needed. Auto resize sounds good too.

@algermissen Glad it's working! Adding Counter for i32 shouldn't really be doing anything important, though. Negative values aren't supported anyway. I tried it with just plain old u64 and interop worked with the JS demo and some throwaway tools I made with the Java impl.

One other thing is that this is a backwards-incompatible change if people have caches of old histograms serialized with the plain deflate compression. If that is an issue for people we can resuscitate the old form to let them transcode the old form into the new form.

@@ -1,57 +1,61 @@
extern crate hdrsample;
#[cfg(feature = "serialization")]
#[cfg(test)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use all(feature = "serialization", test) here I believe

@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

Ah, yes, that came as a result of #56.

For the example binary, we should use required-features instead of this nasty whole-module cfg gate.

As for backwards compatibility, I think we should make the change, and then provide a fix for those in need if complaints should arise.

Also, rename the example to `cli` and use a subcommand since I already
have other ideas for things to add.
@jonhoo
Copy link
Collaborator

jonhoo commented Oct 10, 2017

@marshallpierce if you think we might want to add other things, perhaps this should become a bin, and not just an example?

@marshallpierce
Copy link
Collaborator Author

Not a bad idea, but I think I'd like to incubate things a bit as an example before we ship it as something semi-official.

@jonhoo jonhoo merged commit d6b3384 into master Oct 10, 2017
jonhoo added a commit that referenced this pull request Oct 10, 2017
Note a slight backwards incompatibility for serialized histograms:
#63 (comment)
@marshallpierce marshallpierce deleted the serialization-compat branch November 1, 2017 01:45
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.

3 participants