Skip to content

Clojure Protobuf#12

Open
milt wants to merge 34 commits intobluemont:masterfrom
yetanalytics:clj_pb
Open

Clojure Protobuf#12
milt wants to merge 34 commits intobluemont:masterfrom
yetanalytics:clj_pb

Conversation

@milt
Copy link
Contributor

@milt milt commented Feb 26, 2016

While I understand (and agree with) the rationale (in the README) for not using lein-protobuf, it's companion library clj-protobuf seems like it might be a pretty good fit to replace the code currently under /src/clojure/kria/pb, favoring a data-driven approach to constructing messages.

This pull demonstrates the usage of clojure-protobuf with the same riak-pb compiled java provided by Basho, but without any of the direct interop formerly found in the pb namspaces.

Pros

  • Much easier to maintain (net of 1291 LOC removed)
  • Better, schema-aware errors
  • Protodefs can be queried for their schema
  • All args are just clojure data (with the exception of byte-string literals), no mapping Pair->pb over stuff.

Cons

  • clojure-protobuf uses some reflection to build messages, which can slow things down slightly when encoding/decoding complex messages.
  • A bit more dependency-heavy

Breaking Changes

There are some small breaking changes to the API, namely every api fn that formerly took plain clojure string args has been changed to require pb byte-strings instead. This eases the burden of coercion on subsequent calls, for instance using Links, etc, and makes the API consistent.

Parsing/Dispatch

kria.core/call no longer takes the req->bytes and bytes->resp fns, dispatching instead on the request/response keys given. This also removes the need to supply no-ops like (fn [_] true) and (fn [_] (byte-array 0)) in api functions, as these are provided when there is no definition found for the message. Clojure-protobuf has the added benefit of more human-readable exceptions when it gets a bad message to write, which should ease use considerably, with direct interop I found myself hunting down NPE's a lot.

The tests only required minor changes mostly concerning api functions that formerly took strings.

Further Work

I plan to do more investigation into the best way to declare/call protobuf definitions, you can see the way I am currently doing it in the pb namespaces.

@xpe
Copy link
Contributor

xpe commented Mar 23, 2017

Hello, thank you for the PR. Somehow I overlooked this PR.

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.

2 participants