Null Value holder change across collection cache and address cache layer #6312
Conversation
kushagraThapar
commented
Nov 13, 2019
•
kushagraThapar
commented
- Updated collection layer logic with same as V2 SDK
- Removed Mono.empty() and Flux.empty() where V2 SDK was returning null values, instead used a ValueHolder<>(null)
- Addresses the issue: Consistency Layer and collection cache optional null value change #6307
…d Mono.empty() and instead used ValueHolder<>(null)
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
...mos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/caches/RxCollectionCache.java
Show resolved
Hide resolved
...re-cosmos/src/main/java/com/azure/cosmos/implementation/caches/RxPartitionKeyRangeCache.java
Show resolved
Hide resolved
|
/azp run java - cosmos - tests |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| collectionResourceId, | ||
| null, | ||
| () -> this.getByRidAsync(collectionResourceId, properties)); | ||
| return async.map(Utils.ValueHolder::new); |
There was a problem hiding this comment.
async.map(Utils.ValueHolder::new) seems odd.
If async Mono may return a null value, then by the time the follow has reached map(Utils.ValueHolder::new); Mono will explode.
if async Mono can never return a null value, then why doing async.map(Utils.ValueHolder::new); at all?
There was a problem hiding this comment.
Good point, but async Mono can never return a null value.
The reason for transforming the the async value to Utils.ValueHolder is because of the fact that whoever calls this method also returns null values in some other cases.
If we don't do the transformation here, we will have to do the transformation at the caller level, and there are multiple callers, so that will be code duplication.
So in my opinion it is better to do the transformation at one place than everywhere else.
The same comment applies to below comments as well.
|
|
||
| return Mono.error(err); | ||
| }); | ||
| .map(Utils.ValueHolder::new) |
There was a problem hiding this comment.
if the upstream can generate null, the upstream Mono will explode before reaching .map(Utils.ValueHolder::new)
if the upstream cannot generate null value then why adding this transformation here in the first place?
| collectionRid, | ||
| null, | ||
| () -> getRoutingMapForCollectionAsync(collectionRid, null, properties)); | ||
| () -> getRoutingMapForCollectionAsync(collectionRid, null, properties)).map(Utils.ValueHolder::new); |
There was a problem hiding this comment.
if the upstream can generate null, the upstream Mono will explode before reaching .map(Utils.ValueHolder::new)
if the upstream cannot generate null value then why adding this transformation here in the first place?
...cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/AddressResolver.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@kushagraThapar we are merging this PR in the interest of time. The comments on this PR needs to be addressed as soon as possible as a priority immediately after the release.
