search

NicolasT / kontiki

An implementation of the Raft consensus protocol
BSD 3-Clause "New" or "Revised" License
122 stars 15 forks source link

Changes to build and pass tests against Haskell platform 2013.2 #1

Closed hargettp closed 11 years ago

hargettp commented 11 years ago

Not sure if these changes are too radical, as they do replace Builder with strict ByteString in the CLog constructor. Nevertheless, building on Haskell Platform might help this code find wider use.

Built and tested using cabal-dev --enable-tests install. Biggest change was dropping the version restriction on bytestring, and adding one on binary. A consequence of that change is that the CLog constructor now takes a strict ByteString, rather than a Builder.

Tested on OS X Mountain Lion with Haskell Platform 2013.2.

Tested on Ubuntu 12.04 x86-x64 using GHC 7.6.3 and Cabal 1.16.0.2, as no actual Haskell Platform build was available.

NicolasT commented 11 years ago

The use of Builder was intentional, since we noticed in several production apps logging, and especially 'expanding' log messages, can have a major performance impact, which is useless when the log messages aren't used after all (e.g. because the log level is set at some high level).

The version bound on bytestring was there because in new bytestring releases, Builder is no longer in Data.ByteString.Lazy.Builder, so for forwards compatibility I did prefer to put a higher bound. I guess this is open for debate though :-)

hargettp commented 11 years ago

Hey, it's not really up for debate, as it is YOUR code. :)

Thanks for explaining why the conscious choice of using Builder. Sounds like the choice was made carefully. Certainly in light of real-world conditions the constraint of compatibility with Haskell Platform is an artificial one.

Anyway, thanks for the response!

:)

On Aug 10, 2013, at 11:53 AM, Nicolas Trangez notifications@github.com wrote:

The use of Builder was intentional, since we noticed in several production apps logging, and especially 'expanding' log messages, can have a major performance impact, which is useless when the log messages aren't used after all (e.g. because the log level is set at some high level).

The version bound on bytestring was there because in new bytestringreleases, Builder is no longer in Data.ByteString.Lazy.Builder, so for forwards compatibility I did prefer to put a higher bound. I guess this is open for debate though :-)

— Reply to this email directly or view it on GitHubhttps://github.com/NicolasT/kontiki/pull/1#issuecomment-22442111 .

NicolasT commented 11 years ago

Don't be mistaken, this is not about being 'my code' whatsoever ;-)

If you'd be willing to adapt your patch so it works with bytestring as shipped with HP, as well as the latest releases (e.g. by wrapping imports with CPP), assuming that's possible (didn't give it much thought, sorry), I'd be happy to accept the change!