> On Dec. 10, 2013, 9:06 p.m., Jay Kreps wrote: > > I actually thought it made sense to have it in the Serialization level > > since null is about the serialization of the objects--i.e. that is the > > level that translates from the user's understanding to the system > > representation (bytes and null for delete). What did it break?
Documented in more detail on SAMZA-104, but the core problem is that CacheStore wraps SerializedKeyValueStore, and CacheStore.delete calls store.put(k, null). This triggers an NPE on all deletes, since the serde store turns around and checks that the value should NEVER be null. Another option would be to have CacheStore.delete not put a null value into the cache (which then gets flushed and triggers the NPE), but instead call delete on the underlying store directly. This approach seemed really complicated since it means managing batch deletes separately from normal puts when flush is called. It also was a little unclear to me what the impact would be to this approach on the cache: does store.delete mean we should delete the CacheEntry? Seems not, since doing so could delete dirty entries that haven't been flushed (thereby losing complete write history for a key). Maybe losing intermediate writes is OK if we know a delete has been written afterwards, maybe not. Anyway, that approach's complexity started to concern me. - Chris ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16159/#review30127 ----------------------------------------------------------- On Dec. 10, 2013, 6:47 p.m., Chris Riccomini wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16159/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2013, 6:47 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > create a null safe key value store, and remove all null checks from serde > store > > > create a test that exposes the bug. > > > Diffs > ----- > > > samza-kv/src/main/scala/org/apache/samza/storage/kv/KeyValueStorageEngineFactory.scala > d42f46ec59b70bbff5814862f3b86dab0195502e > > samza-kv/src/main/scala/org/apache/samza/storage/kv/NullSafeKeyValueStore.scala > PRE-CREATION > > samza-kv/src/main/scala/org/apache/samza/storage/kv/SerializedKeyValueStore.scala > c3bf4dcc37718ef57e4332e5daa9a6725b5b0739 > > samza-kv/src/test/scala/org/apache/samza/storage/kv/TestKeyValueStores.scala > e8db3f2111f1bfa922b4014b828c41d0c61a7151 > > Diff: https://reviews.apache.org/r/16159/diff/ > > > Testing > ------- > > > Thanks, > > Chris Riccomini > >
