standalone deser #239

noisesmith · 2020-04-02T23:36:05Z

Updates some stale comments.

Changes the spec for avro serdes so that the schema document is optional. This allows a reader to continue using a topic as the producer evolves their schema, without updating the schemas documents in lockstep.

Checklist

tests
updated CHANGELOG (the "unreleased" section)

MicahElliott · 2020-04-02T23:38:54Z

src/jackdaw/specs.clj

+    :opt-un [::schema
+             ::schema-filename]
+    :req-un [::serde-keyword
+             ::key?]))


Maybe worth have the req-un before that opt-un. I feel I see that most commonly.

noisesmith · 2020-04-02T23:59:52Z

I'm still working on the test coverage and CHANGELOG update

creese · 2020-04-03T00:26:13Z

We may want to create a separate top-level function for when a deserializer-only serde is wanted. That feels a bit safer to me than relaxing the current spec.

codecov · 2020-04-03T14:25:31Z

Codecov Report

Merging #239 into master will decrease coverage by 0.27%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master     #239      +/-   ##
==========================================
- Coverage   80.30%   80.02%   -0.28%     
==========================================
  Files          42       42              
  Lines        2660     2673      +13     
  Branches      153      153              
==========================================
+ Hits         2136     2139       +3     
- Misses        371      381      +10     
  Partials      153      153

Impacted Files	Coverage Δ
src/jackdaw/serdes/resolver.clj	`90.62% <75.00%> (+0.30%)`	⬆️
src/jackdaw/specs.clj	`78.78% <88.88%> (+5.71%)`	⬆️
src/jackdaw/serdes/avro.clj	`89.07% <100.00%> (+0.12%)`	⬆️
src/jackdaw/serdes/avro/confluent.clj	`100.00% <100.00%> (ø)`
src/jackdaw/serdes/edn2.clj	`45.45% <0.00%> (-45.46%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ae7dd6...6df5f40. Read the comment docs.

noisesmith · 2020-04-03T16:12:28Z

We may want to create a separate top-level function for when a deserializer-only serde is wanted. That feels a bit safer to me than relaxing the current spec.

Thanks Charles, I think the current version of the PR does what you want.

creese · 2020-04-03T17:32:23Z

src/jackdaw/serdes/avro/confluent.clj

@@ -18,3 +18,16 @@
               {:avro/schema schema
                :key? key?
                :avro/coercion-cache coercion-cache})))
+
+(defn reader-serde
+  "Creates a serde for Avro data with an optional schema.


This shouldn't take a schema.

creese · 2020-04-03T17:35:39Z

src/jackdaw/specs.clj

+    :req-un [::serde-keyword
+             ::key?]
+    :opt-un [::schema
+             ::schema-filename]))


~~::key? is only needed for the Schema Registry. We can probably reuse :jackdaw.serde/serde defined above.~~
Let's modify the spec to take an optional key named ::reader-only?. When true, ::schema, ::schema-filename, and ::key? are not required. When false or missing, only ::key? and one of ::schema and ::schema-filename are required.

creese · 2020-04-03T17:36:40Z

src/jackdaw/serdes/avro/confluent.clj

+  this looser spec (see :jackdaw.serde/confluent-avro-reader-serde and
+  :jackdaw.serde/confluent-avro-serde)."
+  [schema-registry-url schema key? & [opts-map]]
+  (serde schema-registry-url schema key? opts-map))


This should call jsa/serde without a schema.

We can set a value for :reader-only? so the user doesn't have to.

creese · 2020-04-03T17:37:39Z

src/jackdaw/serdes/resolver.clj

            (throw (ex-info "Invalid serde config."
-                            (s/explain-data :jackdaw.serde/confluent-avro-serde serde-config)))
+                            (s/explain-data spec-key serde-config)))


The serde-resolver requires a schema. Can we back out these changes?

it's only the serialization side that uses it, and the current kafka-productionised code looks up everything using the resolver

An alternate approach would be to allow the resolver (or something with a similar API) to return a wrapper with only the deserializer field.

The current approach is compatible with the existing code paths because the expected deserializer field is present and the resolver adds the expected conversions on read, I'm open to other options but would like to limit the scope of the API change. What about creating a wrapper where the deserializer works via the schema registry and the serializer simply throws an informative error?

creese · 2020-04-03T19:13:02Z

src/jackdaw/serdes/resolver.clj

@@ -54,12 +54,16 @@
      (let [schema (cond
                     schema-filename (load-schema serde-config)
                     schema          schema)


How do you plan to handle the case where schema and schema-filename are not supplied?

good catch - my test should have caught that but did not

cond just works that way, but I added an explicit nil for clarity

(ins)user=> (cond true 1) 1 (ins)user=> (cond false 1) nil

cddr · 2020-04-03T19:21:28Z

I thought that this PR #157 meant that schema was no longer required but looking back, I don't see a test that proves it to be fair.

noisesmith · 2020-04-03T23:29:59Z

I thought that this PR #157 meant that schema was no longer required but looking back, I don't see a test that proves it to be fair.

I think this is true - but the spec used by the resolver would still fail, and I think I want that entry point.

creese · 2020-04-06T19:38:06Z

test/jackdaw/serdes/resolver_test.clj

+            ;; by providing :serde/type of :read-only we can allow a serde without local schema
+            reader-avro-config {:serde-keyword :jackdaw.serdes.avro.confluent/serde
+                                :key? false
+                                :serde-type :read-only}


I would rather have this work like :key?.

Example:

{:serde-keyword :jackdaw.serdes.avro.confluent/serde :key? false :read-only? true}

Or if :key? is not required:

{:serde-keyword :jackdaw.serdes.avro.confluent/serde :read-only? true}

creese · 2020-04-06T19:41:35Z

What happened to jackdaw.serdes.avro.confluent/reader-schema?

99-not-out · 2020-04-06T19:51:46Z

src/jackdaw/serdes/resolver.clj

@@ -39,6 +39,8 @@
  Options:
  schema-registry-url - The URL for the schema registry
  type-registry - A mapping per jackdaw.serdes.avro/+base-schema-type-registry+>
+  read-only - Specifies that you will not be using the resulting serializer,
+              and does not require a schema or schema-filename


What will happen if you try to write with a read-only avro serdes?
Would be nice to get a clean error message, as debugging avro error is a black art at the best of times!

I've added an informative message, and a test of that message.

noisesmith · 2020-04-06T20:35:45Z

What happened to jackdaw.serdes.avro.confluent/reader-schema?

With a multi-spec a separate top level definition isn't needed - if the :serde-type keys is present with :read-only as the value, the read-only spec is used. This means that resolver stays simple - it doesn't need to know about any other specs, and the spec itself implements that conditional.

creese · 2020-04-07T19:35:24Z

src/jackdaw/specs.clj

+                   ::key?]))
+
+(s/def :jackdaw.serde/confluent-avro-serde
+  (s/multi-spec avro-serde-type :read-only?))


Can we extend the existing key group?

(s/def :jackdaw.serde/confluent-avro-serde (s/keys :req-un [::serde-keyword ::key? (or ::read-only? ::schema ::schema-filename)])

creese · 2020-04-07T23:29:11Z

src/jackdaw/serdes/avro.clj

@@ -671,7 +674,8 @@
   {:keys [avro/schema
           avro/coercion-cache
           key?
-           deserializer-properties]
+           deserializer-properties


What is this?

it pre-exists this pr, introduced in commt #8551c18c836b7b52e0a0b16b9b041d30b3eb4a0c

creese · 2020-04-07T23:34:11Z

src/jackdaw/specs.clj

+                        ::read-only?)])
+   #(exactly-one-true? (string? (:schema %))
+                       (string? (:schema-filename %))
+                       (true? (:read-only? %)))))


I thought we agreed just to check for the key's presence or absence.

I misunderstood - easy enough to fix

making the doc string match the rename

3353862

noisesmith requested a review from a team as a code owner April 2, 2020 23:36

MicahElliott previously approved these changes Apr 2, 2020

View reviewed changes

noisesmith changed the title ~~Ug 22 standalone deser~~ standalone deser Apr 2, 2020

noisesmith dismissed MicahElliott’s stale review via cb1eb1e April 3, 2020 14:20

noisesmith force-pushed the UG-22-standalone-deser branch from cb1eb1e to 1fbe16e Compare April 3, 2020 16:08

creese reviewed Apr 3, 2020

View reviewed changes

creese reviewed Apr 6, 2020

View reviewed changes

99-not-out reviewed Apr 6, 2020

View reviewed changes

noisesmith force-pushed the UG-22-standalone-deser branch from 4203495 to 8e9460a Compare April 6, 2020 23:32

creese reviewed Apr 7, 2020

View reviewed changes

noisesmith force-pushed the UG-22-standalone-deser branch from 56a1df5 to 3f1db89 Compare April 7, 2020 23:24

creese reviewed Apr 7, 2020

View reviewed changes

noisesmith force-pushed the UG-22-standalone-deser branch from 3f1db89 to 6640303 Compare April 7, 2020 23:42

creese previously approved these changes Apr 8, 2020

View reviewed changes

Dedicated reader serde via the resolver.

6df5f40

noisesmith dismissed creese’s stale review via 6df5f40 April 8, 2020 00:09

noisesmith force-pushed the UG-22-standalone-deser branch from 6640303 to 6df5f40 Compare April 8, 2020 00:09

creese approved these changes Apr 8, 2020

View reviewed changes

noisesmith merged commit 2dab283 into master Apr 8, 2020

noisesmith deleted the UG-22-standalone-deser branch April 8, 2020 17:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

standalone deser #239

standalone deser #239

noisesmith commented Apr 2, 2020 •

edited

Loading

MicahElliott Apr 2, 2020

noisesmith Apr 3, 2020

noisesmith commented Apr 2, 2020

creese commented Apr 3, 2020

codecov bot commented Apr 3, 2020 •

edited

Loading

noisesmith commented Apr 3, 2020

creese Apr 3, 2020

creese Apr 3, 2020 •

edited

Loading

creese Apr 3, 2020

creese Apr 3, 2020

creese Apr 3, 2020

noisesmith Apr 3, 2020

creese Apr 3, 2020

noisesmith Apr 3, 2020

noisesmith Apr 3, 2020

cddr commented Apr 3, 2020

noisesmith commented Apr 3, 2020

creese Apr 6, 2020

creese commented Apr 6, 2020

99-not-out Apr 6, 2020

noisesmith Apr 6, 2020

noisesmith commented Apr 6, 2020

creese Apr 7, 2020 •

edited

Loading

creese Apr 7, 2020

noisesmith Apr 7, 2020

creese Apr 7, 2020

noisesmith Apr 7, 2020

standalone deser #239

standalone deser #239

Conversation

noisesmith commented Apr 2, 2020 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisesmith commented Apr 2, 2020

creese commented Apr 3, 2020

codecov bot commented Apr 3, 2020 • edited Loading

Codecov Report

noisesmith commented Apr 3, 2020

Choose a reason for hiding this comment

creese Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cddr commented Apr 3, 2020

noisesmith commented Apr 3, 2020

Choose a reason for hiding this comment

creese commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisesmith commented Apr 6, 2020

creese Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noisesmith commented Apr 2, 2020 •

edited

Loading

codecov bot commented Apr 3, 2020 •

edited

Loading

creese Apr 3, 2020 •

edited

Loading

creese Apr 7, 2020 •

edited

Loading