Improvements around empty data handling #18
Conversation
There was a problem hiding this comment.
First CR on this - why is SimpleStore closeable? Seems strange that the entire store is closeable vs having some sort of transaction that is instead. What if multiple spots access it? Or is SimpleStore the transaction and consumers must always create a new instance from the factory?
protosimplestore/src/main/java/com/uber/simplestore/proto/impl/SimpleProtoStoreFactory.java
Show resolved
Hide resolved
protosimplestore/src/test/java/com/uber/simplestore/proto/SimpleProtoStoreImplTest.java
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,10 @@ | |||
| syntax = "proto2"; | |||
There was a problem hiding this comment.
proto3 doesn't buy anything on mobile and in fact has downsides.
There was a problem hiding this comment.
That's a bit vague - can you elaborate?
There was a problem hiding this comment.
protocolbuffers/protobuf#272 et al.
protosimplestore/src/test/java/com/uber/simplestore/proto/SimpleProtoStoreImplTest.java
Outdated
Show resolved
Hide resolved
protosimplestore/src/test/java/com/uber/simplestore/proto/SimpleProtoStoreImplTest.java
Outdated
Show resolved
Hide resolved
protosimplestore/src/test/java/com/uber/simplestore/proto/SimpleProtoStoreImplTest.java
Outdated
Show resolved
Hide resolved
simplestore/src/main/java/com/uber/simplestore/SimpleStore.java
Outdated
Show resolved
Hide resolved
| @@ -28,6 +29,7 @@ | |||
| private static final int OPEN = 0; | |||
| private static final int CLOSED = 1; | |||
| private static final int TOMBSTONED = 2; | |||
| private static final byte[] EMPTY_BYTES = new byte[0]; | |||
There was a problem hiding this comment.
This is cleaner than constantly allocating an empty byte array.
There was a problem hiding this comment.
That's not what I meant - why is an empty array preferable over null?
There was a problem hiding this comment.
Futures should not have nulls passed through them just like Observables should not. It is up to the user/data model chose to represent optionality in the model itself instead of the framework requiring everyone everywhere to handle null.
simplestore/src/main/java/com/uber/simplestore/impl/SimpleStoreImpl.java
Outdated
Show resolved
Hide resolved
simplestore/src/test/java/com/uber/simplestore/impl/SimpleStoreImplTest.java
Outdated
Show resolved
Hide resolved
|
* Support cache scope config * Handle empty state properly
Never return null from store methods.
| @@ -55,6 +73,14 @@ public static SimpleProtoStoreImpl create(Context context, String scope, ScopeCo | |||
| SimpleStoreConfig.getComputationExecutor()); | |||
| } | |||
|
|
|||
| @Override | |||
| public ListenableFuture<Boolean> contains(String key) { | |||
| return Futures.transform( | |||
There was a problem hiding this comment.
Will optimize contains down the road.
| return create(context, scope, ScopeConfig.DEFAULT); | ||
| } | ||
|
|
||
| public static SimpleProtoStore create(Context context, String scope, ScopeConfig config) { |
There was a problem hiding this comment.
follow up on discussion if a wrap is appropriate
There was a problem hiding this comment.
following up with some prototyping later
No description provided.