You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chris Riccomini <cr...@apache.org> on 2013/12/10 19:47:16 UTC
Review Request 16159: SAMZA-104
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16159/
-----------------------------------------------------------
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
Re: Review Request 16159: SAMZA-104
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16159/#review30143
-----------------------------------------------------------
Ship it!
Ship It!
- Jay Kreps
On Dec. 10, 2013, 10:44 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, 10:44 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> move notNull to Util class in core.
>
>
> create a null safe key value store, and remove all null checks from serde store
>
>
> create a test that exposes the bug.
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 83863247901daab2209d3b23696c8e8af1711bbe
> 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
>
>
Re: Review Request 16159: SAMZA-104
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16159/
-----------------------------------------------------------
(Updated Dec. 10, 2013, 10:44 p.m.)
Review request for samza.
Repository: samza
Description (updated)
-------
move notNull to Util class in core.
create a null safe key value store, and remove all null checks from serde store
create a test that exposes the bug.
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/util/Util.scala 83863247901daab2209d3b23696c8e8af1711bbe
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
Re: Review Request 16159: SAMZA-104
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16159/#review30138
-----------------------------------------------------------
Ship it!
Looks good. Might be nice to move the notNull method into Utils, though.
- Jay Kreps
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
>
>
Re: Review Request 16159: SAMZA-104
Posted by Chris Riccomini <cr...@apache.org>.
> 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
>
>
Re: Review Request 16159: SAMZA-104
Posted by Jay Kreps <bo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16159/#review30127
-----------------------------------------------------------
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?
- Jay Kreps
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
>
>