persist: add some unit tests #12826

danhhz · 2022-06-01T21:38:34Z

See commits for details. The first commit isn't a test, but it was small so I snuck it in.

Motivation

This PR adds a feature that has not yet been specified.

Testing

This PR has adequate test coverage / QA involvement has been duly considered.

Release notes

This PR includes the following user-facing behavior changes:

N/A

danhhz · 2022-06-01T21:42:41Z

src/persist-client/src/read.rs

-            return Err(Since(self.since.clone()));
-        }
+        let mut machine = self.machine.clone();
+        let () = machine.listen(&as_of).await?;


this change hangs the open_loop benchmark because it creates listeners as_of zero at startup and immediately awaits them before any writes have come in. any ideas how to proceed? everything I've come up with has been awful.

materialize/src/persist-client/examples/open_loop.rs

Lines 441 to 443 in a0ec852

let listen = reader

.listen(Antichain::from_elem(0))

.await

Maybe this is an early sign that the previous behaviour of listen() was more ergonomic? (See my other comment)

aljoscha

Nice optimization! 🙌

Regarding the change in semantics of listen(): what's the reasoning behind that? It doesn't seem to be necessary for the optimization, and the test for the optimization could also be written with the old semantics. I'm asking because it seemed natural to me that snapshot() would block, because it does "get me the entire data up to as_of" while listen() felt more like an async stream where creating the stream at any legal as_of would not block but then updates would only trickle in once they are available.

aljoscha · 2022-06-02T08:30:31Z

src/persist-client/src/lib.rs

+                    "{:?}",
+                    client.open::<Vec<u8>, String, u64, i64>(shard_id).await
+                ),
+                "Err(CodecMismatch { requested: (\"Vec<u8>\", \"String\", \"u64\", \"i64\"), actual: (\"String\", \"String\", \"u64\", \"i64\") })"


Why are you using string comparison instead of something like:

assert_eq!( client .open::<Vec<u8>, String, u64, i64>(shard_id) .await .unwrap_err(), InvalidUsage::CodecMismatch { requested: tpe("Vec<u8>", "String", "u64", "i64",), actual: tpe("String", "String", "u64", "i64",), } );

Where tpe() is a helper I made up. Plus I had to add #[cfg_attr(test, derive(PartialEq, Eq))] on InvalidUsage.

The strings seem somewhat hard to maintain, but there probably is a good reason. 😅

I usually lean toward matching on error message in tests because that's often how they're consumed in production. unwrap_err is a good idea, I simply forgot it existed :). I'll switch to that!

danhhz

Regarding the change in semantics of listen(): what's the reasoning behind that? It doesn't seem to be necessary for the optimization, and the test for the optimization could also be written with the old semantics. I'm asking because it seemed natural to me that snapshot() would block, because it does "get me the entire data up to as_of" while listen() felt more like an async stream where creating the stream at any legal as_of would not block but then updates would only trickle in once they are available.

I'm convinced!

danhhz · 2022-06-02T14:44:09Z

src/persist-client/src/lib.rs

+                    "{:?}",
+                    client.open::<Vec<u8>, String, u64, i64>(shard_id).await
+                ),
+                "Err(CodecMismatch { requested: (\"Vec<u8>\", \"String\", \"u64\", \"i64\"), actual: (\"String\", \"String\", \"u64\", \"i64\") })"


I usually lean toward matching on error message in tests because that's often how they're consumed in production. unwrap_err is a good idea, I simply forgot it existed :). I'll switch to that!

ruchirK

First two commits look good to me, third commit im still reading and its taking me a bit to internalize because I'm slow this morning but don't let that block merging!

ruchirK · 2022-06-02T16:06:15Z

src/persist-client/src/lib.rs

+                "Err(CodecMismatch { requested: (\"String\", \"String\", \"i64\", \"i64\"), actual: (\"String\", \"String\", \"u64\", \"i64\") })"
+            );
+            // We can't test the D param mismatch currently because i64 is literally
+            // the only type that implements both Codec64 and Semigroup right now.


hmmm this is really surprising to me because Semigroup/Monoid should be implemented for the unsigned integers and i guess it just never was a pressing need

opened TimelyDataflow/differential-dataflow#368 which once it merges and we bump differential should let us test the diff param mismatch

oh because u64 will implement Semigroup now? nice!

done, thanks for the timely fix!

danhhz

ready for another look!

danhhz · 2022-06-02T16:22:59Z

src/persist-client/src/read.rs

@@ -644,6 +643,11 @@ mod tests {
        let mut snapshot = read.expect_snapshot(2).await;
        let mut listen = read.expect_listen(0).await;

+        // Manually advance the listener's machine so that it has the latest


I don't love this! some other options:

snapshot currently clones the machine so that the methods can be &self instead of &mut self but I don't think that's super important. removing that clone would happen to make this unnecessary because the listen would inherit the state from the snapshot call

change the listen call to try fetching updated state once if it's not immediately serveable

dunno something else?

looking right now! sorry about dropping this!

I think snapshot mutating the reader makes the most sense to me! intuitively, having a mechanism whereby a reader changes after snapshotting seems to make sense, and it seems like we're doing

// Hack: Keep this method `&self` instead of `&mut self` by cloning the // cached copy of the state, updating it, and throwing it away // afterward.

purely as a means to keep that method &self, which i think might have the rationale that its more like the expectation for the api? I don't know of any stronger reason, and given all of that, I feel like &mut self is a fine way forward!

When I was rewriting this test locally, to fit with the previous (and now unchanged!) semantics of listen, I changed this to first do one next() call on listen, asserted against that. Then made it unreliable, and then fetched the rest of the listen events. Also slightly awkward, but doesn't require calling internal methods or changing signatures. 🤷‍♂️

I think it makes a lot of sense to make snapshot &mut and remove the machine clone (and we should consider doing that independantly), but after reverting my change to also make listen wait for as_of to be available, it seems pretty subtle for the test to rely on the fact that we call snapshot first. went with aljoscha's suggestion

danhhz · 2022-06-09T14:30:01Z

either of you want to take another look at this? if not, I'll resolve these conflicts (and fix my lint issue) and merge

aljoscha

I had a nit and a comment. But I think this is good to merge!

aljoscha · 2022-06-10T06:31:33Z

src/persist-client/src/error.rs

-    /// An update was not beyond the expected lower of the batch
-    UpdateNotBeyondLower {
+    /// An update was not at or beyond the expected lower of the batch
+    UpdateNotAtOrBeyondLower {


super nit: I think timely (and Frank) already understand "beyond" as "not less than", which means "at or greater", in laymans terms.

TIL (and I confirmed frank shares this interpretation)! reverted

aljoscha · 2022-06-10T06:41:30Z

src/persist-client/src/read.rs

@@ -644,6 +643,11 @@ mod tests {
        let mut snapshot = read.expect_snapshot(2).await;
        let mut listen = read.expect_listen(0).await;

+        // Manually advance the listener's machine so that it has the latest


When I was rewriting this test locally, to fit with the previous (and now unchanged!) semantics of listen, I changed this to first do one next() call on listen, asserted against that. Then made it unreliable, and then fetched the rest of the listen events. Also slightly awkward, but doesn't require calling internal methods or changing signatures. 🤷‍♂️

aljoscha · 2022-06-10T08:21:45Z

src/persist-client/src/impl/machine.rs

@@ -200,29 +200,48 @@ where
        }
    }

+    pub async fn listen(&self, as_of: &Antichain<T>) -> Result<Self, Since<T>> {


This is super nit, but: it feels a bit weird to have these methods on Machine and State, because ReadHandle doesn't really have to call them, they're just an additional layer of verification/assertion. We could maybe put that in a comment here or maybe call this verify_listen or sth.

This was started in 12685, but the last mine was left as a TODO to avoid making 12216 rebase. Now that 12216 is in, finish this work.

danhhz

TFTRs!!

danhhz · 2022-06-10T17:39:52Z

src/persist-client/src/error.rs

-    /// An update was not beyond the expected lower of the batch
-    UpdateNotBeyondLower {
+    /// An update was not at or beyond the expected lower of the batch
+    UpdateNotAtOrBeyondLower {


TIL (and I confirmed frank shares this interpretation)! reverted

danhhz · 2022-06-10T17:44:29Z

src/persist-client/src/lib.rs

+                "Err(CodecMismatch { requested: (\"String\", \"String\", \"i64\", \"i64\"), actual: (\"String\", \"String\", \"u64\", \"i64\") })"
+            );
+            // We can't test the D param mismatch currently because i64 is literally
+            // the only type that implements both Codec64 and Semigroup right now.


done, thanks for the timely fix!

danhhz · 2022-06-10T17:50:37Z

src/persist-client/src/impl/machine.rs

@@ -200,29 +200,48 @@ where
        }
    }

+    pub async fn listen(&self, as_of: &Antichain<T>) -> Result<Self, Since<T>> {


danhhz · 2022-06-10T18:02:36Z

src/persist-client/src/read.rs

@@ -644,6 +643,11 @@ mod tests {
        let mut snapshot = read.expect_snapshot(2).await;
        let mut listen = read.expect_listen(0).await;

+        // Manually advance the listener's machine so that it has the latest


I think it makes a lot of sense to make snapshot &mut and remove the machine clone (and we should consider doing that independantly), but after reverting my change to also make listen wait for as_of to be available, it seems pretty subtle for the test to rely on the fact that we call snapshot first. went with aljoscha's suggestion

This adds a performance optimization where a Listener doesn't fetch the latest Consensus state if the one it currently has can serve the next request. A similar thing already was true of SnapshotIter, so also included is a test that covers both.

danhhz requested review from aljoscha and ruchirK June 1, 2022 21:38

danhhz commented Jun 1, 2022

View reviewed changes

aljoscha reviewed Jun 2, 2022

View reviewed changes

danhhz commented Jun 2, 2022

View reviewed changes

ruchirK approved these changes Jun 2, 2022

View reviewed changes

danhhz commented Jun 2, 2022

View reviewed changes

danhhz force-pushed the persist_tests branch from c79695f to c590ef8 Compare June 9, 2022 14:42

aljoscha approved these changes Jun 10, 2022

View reviewed changes

aljoscha reviewed Jun 10, 2022

View reviewed changes

danhhz added 2 commits June 10, 2022 10:52

persist: finish plumbing Indeterminate to remaining public API

6b200f1

This was started in 12685, but the last mine was left as a TODO to avoid making 12216 rebase. Now that 12216 is in, finish this work.

persist: add test coverage for every InvalidUsage error

51ae906

danhhz force-pushed the persist_tests branch from c590ef8 to a8fa9e1 Compare June 10, 2022 18:04

danhhz commented Jun 10, 2022

View reviewed changes

danhhz enabled auto-merge June 10, 2022 18:04

danhhz force-pushed the persist_tests branch from a8fa9e1 to 88a2335 Compare June 10, 2022 18:06

danhhz merged commit fa2dd29 into MaterializeInc:main Jun 10, 2022

danhhz deleted the persist_tests branch June 10, 2022 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persist: add some unit tests #12826

persist: add some unit tests #12826

danhhz commented Jun 1, 2022

danhhz Jun 1, 2022

aljoscha Jun 2, 2022

aljoscha left a comment

aljoscha Jun 2, 2022

danhhz Jun 2, 2022

danhhz left a comment

danhhz Jun 2, 2022

ruchirK left a comment

ruchirK Jun 2, 2022

ruchirK Jun 2, 2022

danhhz Jun 2, 2022

ruchirK Jun 2, 2022

danhhz Jun 10, 2022

danhhz left a comment

danhhz Jun 2, 2022

ruchirK Jun 9, 2022

ruchirK Jun 9, 2022

aljoscha Jun 10, 2022

danhhz Jun 10, 2022

danhhz commented Jun 9, 2022

aljoscha left a comment

aljoscha Jun 10, 2022

danhhz Jun 10, 2022

aljoscha Jun 10, 2022

aljoscha Jun 10, 2022 •

edited

Loading

danhhz Jun 10, 2022

danhhz left a comment

danhhz Jun 10, 2022

danhhz Jun 10, 2022

danhhz Jun 10, 2022

danhhz Jun 10, 2022

persist: add some unit tests #12826

persist: add some unit tests #12826

Conversation

danhhz commented Jun 1, 2022

Motivation

Testing

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljoscha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhhz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruchirK left a 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

danhhz left a 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

danhhz commented Jun 9, 2022

aljoscha left a 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

aljoscha Jun 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danhhz left a 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

aljoscha Jun 10, 2022 •

edited

Loading